Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TS: Rely on new web-stream-tools types, fix SignOptions #1502

Merged

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Mar 1, 2022

Also:

TODO:

  • point to released web-stream-tools version

@larabr larabr requested a review from twiss March 1, 2022 11:58
@larabr larabr marked this pull request as ready for review March 3, 2022 12:29
@edmorley
Copy link

edmorley commented Sep 9, 2022

@larabr @twiss Hi! :-)

After this change I now get:

node_modules/openpgp/openpgp.d.ts:10:85 - error TS2307: Cannot find module '@openpgp/web-stream-tools' or its corresponding type declarations.

10 import type { WebStream as GenericWebStream, NodeStream as GenericNodeStream } from '@openpgp/web-stream-tools';
                                                                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Found 1 error in node_modules/openpgp/openpgp.d.ts:10

Should @openpgp/web-stream-tools have been added to dependencies not devDependencies in this repo?

Alternatively, could you document the required types packages in the README?

@larabr
Copy link
Collaborator Author

larabr commented Sep 12, 2022

Hey @edmorley , thanks for spotting this. I wouldn't add it to the dependencies since it's only required by TS, which we do not officially support .

Adding the info on the readme is a good idea, feel free to open a PR :)

@KnisterPeter
Copy link
Contributor

@larabr I find it really strange that you do not support typescript 'officially' but do provide typings with this package. And then refuse to fix issues with that.

@larabr
Copy link
Collaborator Author

larabr commented Jan 27, 2023

@KnisterPeter We provide typings because we think it adds value, and it's better than the alternative, namely relying on https://github.com/DefinitelyTyped/DefinitelyTyped, which was done in previous versions, and was problematic since we weren't directly checking them for correctness. This specific issue is also easily fixed on the app's side.
That said, we're planning to add full TS support in future versions of openpgpjs.

@KnisterPeter
Copy link
Contributor

KnisterPeter commented Jan 27, 2023

@larabr Thanks for your answer.

We provide typings because we think it adds value

Currently it's broken and it's not easy to see how this could be fixed (besides searching all issues). It breaks the npm dependency chain.
Anyway I will for sure accept it.

That said, we're planning to add full TS support in future versions of openpgpjs

Good to hear 😃

@larabr
Copy link
Collaborator Author

larabr commented Jan 27, 2023

I agree searching issues is not ideal, which is why we would welcome a PR that adds info about how to integrate Typescript in the README :)
We just have a lot on our hands at the moment, and contributing to smaller issues like this really makes a difference.

@KnisterPeter
Copy link
Contributor

Here we go: #1586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message.getText() returns a ReadableStream that is inconsistent with typedefs
4 participants