Openpgpjs: Message.getText() returns a ReadableStream that is inconsistent with typedefs

[alichry]

When calling getText() on a Message instance, the expected return type is a string, openpgp.Stream<Uint8Array>, or null. Instead, an instance of ReadableStream was returned.

See:

• openpgp.d.ts#L232
• message.js#L288
• packet/literal_data.js#L65
• openpgpjs/web-stream-tools/streams.js#L264

OpenPGP v5.0.0-1

Snippet:

let test4 = async () => {
    const plain = "hello";
    const message1 = openpgp.Message.fromText(plain);
    const armor = message1.armor();
    console.log("Armored: ");
    console.log(armor);

    const message2 = await openpgp.readMessage({ armoredMessage: armor });
    const text = message2.getText()// as globalThis.ReadableStream;
    if (null === text) {
        console.log("getText() returned null :(");
        return;
    }
    if (typeof text === "string") {
        console.log("getText() returned a string:", text);
        return;
    }
    if (typeof text.read !== "function") {
        console.log("Returned object does not have a method named 'read'!");
    }
    console.log("Assuming returned object is an instance of ReadableStream");
    const reader = text.getReader();
    console.log("result:");
    console.log(await reader.read());
}

[jvalkeal]

I'm not sure but this might have been fixed in main branch. I had something very similar and it worked with type changes after last preview release.

[twiss]

It's not really inconsistent, just underspecified; as Stream is meant to model ReadableStream (in addition to NodeStream). This also applies more generally to top-level functions like openpgp.decrypt, which returns a ReadableStream if you pass one to readMessage, but we return a WebStream according to the types, which is a simplified version of ReadableStream. This is more or less intentional, as the type definitions say: https://github.com/openpgpjs/openpgpjs/blob/39aa742c7ab5a61f07bcf30fb7e3daa34ae8ad8e/openpgp.d.ts#L482
It's not super obvious how to fix this - to return an actual ReadableStream, we'd have to do something like

/// <reference lib="dom" />

but that would cause the entire DOM to be globally available for all users of the library, as far as I understand (correct me if I'm wrong). Whereas really we only want to reference ReadableStream if it exists.

Another option would be to return the same type as what was passed, but then the issue is that we can't express anymore the fact that the returned stream might have a different type of contents than the passed stream. What would be really nice is if we could say something like

export function decrypt<T extends ReadableStream | NodeStream>(options: DecryptOptions & { message: Message<T<Uint8Array | string>>, format: 'binary' }): Promise<DecryptMessageResult & { data: T<Uint8Array> }>
export function decrypt<T extends ReadableStream | NodeStream>(options: DecryptOptions & { message: Message<T<Uint8Array | string>> }): Promise<DecryptMessageResult & { data: T<string> }>

but I don't believe this is legal :)

A third option would be to copy more of the ReadableStream definition into the type definitions, or import them from the polyfill that we already include (that still leaves the question of what to do with NodeStream, though).

The fourth option is to leave things as they are for now, and declare that the as ReadableStream is not so bad :) We can always fix it in the future without breaking backwards compatibility, afaict.

A fifth option would be to have separate type definitions for the web version and the Node.js version of OpenPGP.js. That might honestly be the cleanest solution, but would be some extra work.

Opinions? (@tomholub?)

[tomholub]

I did notice that the stream related APIs were sometimes weirdly inconvenient (even for non-stream use cases) so I'd have to as ThisOrThat it here and there which is generally somewhat bad but obviously miles ahead of not having types at all.

export function decrypt<T extends ReadableStream | NodeStream>(options: DecryptOptions & { message: Message<T<Uint8Array | string>>, format: 'binary' }): Promise<DecryptMessageResult & { data: T<Uint8Array> }>
export function decrypt<T extends ReadableStream | NodeStream>(options: DecryptOptions & { message: Message<T<Uint8Array | string>> }): Promise<DecryptMessageResult & { data: T<string> }>

but I don't believe this is legal :)

Sometimes it's possible to bend and twist TypeScript to express a magic/flexible peace of JS. And sometimes not, and if one cares about the TypeScript usecase a lot, the JS will have to be bent to fit the TS.

If mountain won't go to Muhammad, then Muhammad has to go to the mountain :)

Depending on what exactly the status of v5 release is right now, there may be an opportunity to split a method in two to address whatever the inconvenience. I don't know if it's decrypt vs decryptBinary or if it's decrypt vs decryptStream or what is the angle causing friction.

Other than that, if providing different types to Node and web environment is possible, and it solves the friction, that would be very nice.

[tomholub]

Possibly, instead of splitting the JavaScript, you may be able to define more overloads in TS, causing each definition to be less clever, but more verbose overall.

decrypt(web stream) -> web stream<xx>
decrypt(web stream, binary) -> web stream<yy>
decrypt(node stream) -> node stream<xx>
decrypt(node stream, binary) -> node stream<yy>
decrypt(string) -> string
decrypt(bytes, binary) -> bytes

and so on until all permutations are clearly defined. This should mean that users no longer have to as Xxx as well, but without actually trying this, I don't know if it addresses the problem.

[twiss]

Well - if we want to eliminate casting entirely, we'd have to return a ReadableStream rather than a WebStream (at least as it's currently defined) - so the question is still, how do we say -> ReadableStream<string> without having a reference to ReadableStream. I don't think we can, so we would have to have:

// openpgp.d.ts:
decrypt(Data) -> string
decrypt(Data, binary) -> Uint8Array

// openpgp.web.d.ts:
/// <reference lib="dom" />
decrypt(ReadableStream<Data>) -> ReadableStream<string>
decrypt(ReadableStream<Data>, binary) -> ReadableStream<Uint8Array>

// openpgp.node.d.ts:
/// reference something else?
decrypt(stream.Readable<Data>) -> stream.Readable<string>
decrypt(stream.Readable<Data>, binary) -> stream.Readable<Uint8Array>

[alichry]

Hello @twiss @tomholub 👋

It's not really inconsistent, just underspecified; as Stream is meant to model ReadableStream (in addition to NodeStream).

What I meant by inconsistent is that I was not able to call .read(), since read was undefined, on the returned stream. I had to run .getReader().read() instead.

https://github.com/openpgpjs/openpgpjs/blob/39aa742c7ab5a61f07bcf30fb7e3daa34ae8ad8e/openpgp.d.ts#L482-L491

I personally do not see harm in going with the third option :)

[twiss]

Right, but the definitions you quoted say that Stream is either a WebStream or a NodeStream. In this case it happens to be a WebStream, which indeed doesn't have a read function. So the definitions could be more specific, but they don't promise that you can call read, as written.

[alichry]

Oh, that makes sense 👍 thank you for the clarification. I need to practice proofreading a bit more