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

Get TypeScript type definitions up to date and working again #561

Closed
LucasHill opened this issue Jul 13, 2017 · 57 comments
Closed

Get TypeScript type definitions up to date and working again #561

LucasHill opened this issue Jul 13, 2017 · 57 comments
Milestone

Comments

@LucasHill
Copy link

Sorry if this is not desired here or just the wrong place, but I wanted to bring up an issue over at the DefinitelyTyped project. Openpgpjs has type definitions there but they are not working. I am using openpgpjs in my own TypeScript project and not having the type definitions is a bummer. I would be willing to look into it myself but I am pretty new at TypeScript.

The current implementation is also not in the correct format I believe as there are no exports from the index.d.ts.

@bartbutler
Copy link
Member

Hi Lucas,

Unfortunately, I also have very little experience with Typescript. If you'd like to submit a fix for this I'd be happy to merge it.

@CB-Giepa
Copy link

I also need the updated Typings for the libary, so I am currently putting some work into that.
Unfortunately I have some trouble to understand the Documentation. The return values of the functions are for me the most tricky part.

For example the function encrypt(...) in the openpgp module returns a "encrypted (and optional signed message)". So that is another message object than from the function encryptSessionKey?

Hopefully someone can help me to handle with this.
Greetings.

@bartbutler
Copy link
Member

Hi @CB-Giepa,

So you should think of an OpenPGP message as sort of a container. Most encrypted messages will have an encrypted data packet and one or more key packets for the recipients. encryptSessionKey's Message object just has key packets. The data packet is then added by the rest of 'encrypt' function. encryptSessionKey is exposed as well as encrypt because it can be useful to manipulate the (small) key packets separately from the (large) data packet.

@ffflorian
Copy link

ffflorian commented Sep 18, 2017

Hi @CB-Giepa,
is there any update on the progress? I'd love to use OpenPGP in TypeScript 🙂. Do you need any help?

@BrunoScheufler
Copy link

Sadly this issue seems a bit abandoned, is there any progress of creating updated type definitions?

@bartbutler
Copy link
Member

It would be great to have someone from the community take this on now that OpenPGP.js 3 is out. None of the core devs have much experience with TypeScript.

@tomholub
Copy link
Contributor

tomholub commented May 5, 2018

On behalf of @FlowCrypt I'll send a $100 bounty to the author of the first accepted PR.

@errietta
Copy link

errietta commented Jun 5, 2018

I got mine merged in!
DefinitelyTyped/DefinitelyTyped#26231

Hopefully they are good, they were good enough for my use case.

@up4
Copy link

up4 commented Jun 10, 2018

Hi @errietta ! I just tried your types in the context of StencilJS (a web framework similar to Vue, etc.) and in the IDE (VSCode) I'm trying to import it like so (after npm i openpgp @types/openpgp) : import { generateKey } from "openpgp";. There is no error in the IDE but the transpile rollup dies with this error : 'generateKey' is not exported by node_modules/openpgp/dist/openpgp.js.

I don't know if I'm doing this right. Can anybody help me ? Thanks.

@errietta
Copy link

Hi @up4 . That seems fine for me. Which version of @types/openpgp do you have, you need 0.0.30

@up4
Copy link

up4 commented Jun 10, 2018

Hi @errietta ! Thanks for your reply ! I got it working by looking at your code demos and using import openpgp from "openpgp"; and openpgp.generateKey({…}); instead.

Also : good job !!! Thanks for taking the time to do this !

@errietta
Copy link

It's interesting, both work for me, thanks for the feedback though, glad you got it working 🎉 😄

@tomholub
Copy link
Contributor

@errietta thanks for the PR! Do you want to claim the bounty?

@tomholub
Copy link
Contributor

Spotted a mistake:

// from types
function digest(algo: enums.hash, data: string): string;
// try in node
let openpgp = require('openpgp');
openpgp.crypto.hash.digest(openpgp.enums.hash.sha1, 'test');

outputs:

Uint8Array [
  169,
  74,
  143,
  229,
  204,
  177,
  155,
  166,
  28,
  76,
  8,
  115,
  211,
  145,
  233,
  135,
  152,
  47,
  187,
  211 ]

Types show a string but it's actually Uint8Array returned.

@tomholub
Copy link
Contributor

@errietta Also, I found that declaring the types as follows:

declare module 'openpgp' {
    export interface ...
    ...
}

Allows me to import it as follows:

import * as openpgp from 'openpgp';

without installing @types/openpgp. This is useful for development of the type definitions.

How did you convince TS to use the definitions locally in the current form?

@errietta
Copy link

@tomholub You can test locally using these instructions - https://github.com/DefinitelyTyped/DefinitelyTyped#test-editing-an-existing-package
Do you want me to take a look at that Uint8Array problem?

@tomholub
Copy link
Contributor

Thanks, I'll try that.

There's actually more issues: v3 functions often return promises, but this is not always reflected in the definitions, eg this:

export async function generate(options) {

Is an async function which always returns a promise, but here:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/openpgp/index.d.ts#L514

It's not showing a promise.

I guess I'll just be fixing these issues on my end as I discover them, and make another pull request later to address these.

You can still claim the bounty if you like, the contribution is helpful as is, although not completely consistent with the api.

@errietta-fairfx
Copy link

@tomholub thank you for this, I'm using this on my project as well, so it's good to have them fixed 😄
About the bounty, I don't care about it, if you really insist you can donate here for me - https://www.mind.org.uk/ - thanks a lot though!

@tomholub
Copy link
Contributor

image

@errietta
Copy link

Sweet @tomholub thanks a lot 👍 🎉

@tomholub
Copy link
Contributor

I'm still working on this. You can track progress at FlowCrypt/types#1 & feel free to use updated types from the repo. When I'm satisfied with it, I'll make another PR.

@ffflorian
Copy link

I cleaned up the code and fixed some return types: DefinitelyTyped/DefinitelyTyped#27609

@tomholub
Copy link
Contributor

@ffflorian did you include changes from FlowCrypt/types#1 ? There were loads of stuff missing.

@ffflorian
Copy link

@tomholub I saw that too late, sorry. Will hold the PR until I merged your changes.

@tomholub
Copy link
Contributor

tomholub commented Jul 26, 2018 via email

@tomholub
Copy link
Contributor

tomholub commented Jan 2, 2019

I have further updated the definitions at https://github.com/FlowCrypt/types/blob/master/openpgp.d.ts to reflect OpenPGP v4 api, streaming, etc. The improvements are ongoing

It would be better to include TypeScript API definitions in this repository. That way we can keep the changes easily in sync. Also, nodejs users would automatically get type definitions when they npm install openpgp instead of having to then npm install @types/openpgp where the match between the library and definitions is questionable. When done, we should let folks at DefinitelyTyped know that we'll maintain the definitions here going forward.

I'm ok to personally keep the definitions up to date since our company is heavily invested in TypeScript.

@errietta
Copy link

errietta commented Jan 2, 2019

@tomholub so, we have that repository, and also https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/openpgp/index.d.ts, is there any idea as to what we can do about not keeping both diverging? 😂

@duggabe
Copy link

duggabe commented Feb 9, 2019

@errietta I am new to Angular and have been using openpgp with nodejs successfully. I am porting the openpgp functions to Angular, but am having the problems discussed in this issue.
My package.json has
"@types/openpgp": "^4.0.4", and
"openpgp": "^4.4.7"
in Angular2 I get an error:
TS2305: Module '"/Users/barry/Angular2/keyManager/node_modules/@types/openpgp/ts3.2"' has no exported member 'openpgp'. Can you point me in the right direction?
Thanks!

@errietta
Copy link

errietta commented Feb 9, 2019

@duggabe how are you importing?

Youmay need to do this:

import * as openpgp from 'openpgp';

@duggabe
Copy link

duggabe commented Feb 9, 2019

@errietta We're getting there, but now there seems to be a path problem:
13:29:53 - File change detected. Starting incremental compilation...
[0]
[0] 13:29:53 - Found 0 errors. Watching for file changes.
[1] [Browsersync] Reloading Browsers...
[1] 19.02.09 13:29:54 200 GET /index.html
...
[1] 19.02.09 13:29:55 304 GET /rxjs/Observer.js
[1] 19.02.09 13:29:55 404 GET /openpgp <- note the 404 error
[1] 19.02.09 13:29:56 304 GET /rxjs/util/noop.js

What now? I really appreciate your help!

@tomholub
Copy link
Contributor

tomholub commented Feb 9, 2019

@errietta I do believe I have received a goahead on including TS definitions in this repo from the maintainers.. somewhere.. forgot where. I'm pretty sure they will be down with it.

@tomholub
Copy link
Contributor

tomholub commented Feb 9, 2019

There are a few more updates I wanted to make, and then I think it'll be ready for a PR to merge here.

@tomholub
Copy link
Contributor

tomholub commented Feb 9, 2019

@duggabe this is probably out of scope for this GitHub repo, but you seem to not be including '.js' there (or a full path to the library). You'll have to figure out what the proper way to include 3rd party is in whatever environment / build mechanism you are using.

@duggabe
Copy link

duggabe commented Feb 9, 2019

@errietta and @tomholub I agree this is off-topic for this issue. However, it would be extremely helpful to others if the documentation included Angular aspects such as the import * as openpgp from 'openpgp';.
Should I open an issue on that?
Thank you both for your help and comments.

@tomholub
Copy link
Contributor

Importing openpgp this way is not really an Angular thing, I think it's typescript preferred way to import. We could keep this in mind - once we add TypeScript as part of this repo, maybe the docs can touch on TS support and how to import the lib.

@duggabe
Copy link

duggabe commented Feb 12, 2019

@errietta Here is another related types issue:
var hkp = new openpgp.HKP ('https://pgp.mit.edu'); produces Property 'HKP' does not exist on type 'typeof import("/Users/barry/Angular2/keyManager/node_modules/@types/openpgp/ts3.2/index"
Any ideas?

@tomholub
Copy link
Contributor

Looks like missing definitions

@duggabe
Copy link

duggabe commented Feb 21, 2019

@errietta do you have unreleased HKP type defs I could patch into index.d.ts to try? Have you or any of the others used the HKP functions in TypeScript?

@errietta
Copy link

@duggabe no, I've got nothing unreleased. I've not tried it so it may well not be covered 😞 I can certainly take a look later though..

@duggabe
Copy link

duggabe commented Feb 21, 2019

@errietta OK thanks. Once you have something you want to try, I have a good platform to test it.

@errietta
Copy link

@duggabe
Copy link

duggabe commented Feb 26, 2019

@errietta Thanks, that fixed the HKP class def. Should there have been a types/openpgp/common folder? I had to make one in order to create the index.d.ts in it.
Now I am getting SyntaxError: expected expression, got '<' (http://localhost:4200/node_modules/openpgp/dist/openpgp.worker.min.js:1) Is that something the team needs to look at?

@duggabe
Copy link

duggabe commented Mar 19, 2019

@errietta I have loaded version 4.4.10. The HKP definitions are fixed. The 'hkp.lookup' would work except for a CORS problem. The 'hkp.upload' still produces the error above. I looked at the code, and I don't understand it enough to know what's wrong.
FYI, I have the beginnings of a PGP key manager at https://github.com/duggabe/pgp_key_manager_ts that I have been using for testing.
Please let me know if there is anything I can do to help. Thanks.

@theaccordance
Copy link

Is anyone else having issues with the type definition for cleartext.fromText? I think the definition is incorrect but I wanted to double check before going through the effort of forking/contributing/submitting to DefinitelyTyped.

The source code for the fromText method returns an instance of CleartextMessage , but in the definition returns a type void

@tomholub
Copy link
Contributor

Take a look at https://github.com/FlowCrypt/types/blob/master/openpgp.d.ts (I'm on mobile now). Does it look better?

@tomholub
Copy link
Contributor

My main hesitation about working on getting my types (which I maintained) merged into OpenPGP.js is that they are incompatible with the current types published DefinitelyTyped. But since there will be a breaking v5 release, it will be worth to do the types as well in the same step.

So @twiss if you put this into a v5 milestone then someone on our team will work on updating my types from https://github.com/FlowCrypt/types/blob/master/openpgp.d.ts to match v5 structure and we can have the types here going forward. I don't know when the release for v5 is planned but sometime November I should have someone free enough to do this.

@twiss twiss added this to the v5 milestone Oct 19, 2020
@twiss
Copy link
Member

twiss commented Oct 19, 2020

That would be great, thanks! We will start making prereleases soon, but won't make the final release before November. I think we can wait for this with that.

@twiss
Copy link
Member

twiss commented Dec 9, 2020

@tomholub Gentle ping, do you guys have bandwidth for this? I would like to merge the v5 branch into master sometime soon, and hopefully release v5 this month.

@tomholub
Copy link
Contributor

tomholub commented Dec 9, 2020 via email

@twiss
Copy link
Member

twiss commented Dec 9, 2020

Sounds good, thanks! A high-level changelog (including examples) is here: https://github.com/openpgpjs/openpgpjs/wiki/V5-Changelog

After merging the v5 branch I'll update the documentation as well.

@tomholub
Copy link
Contributor

This has been merged to v5 branch. Although it's not released to users yet, it may make sense to close this issue & track any improvements that come up in separate issues later.

@twiss
Copy link
Member

twiss commented Jan 31, 2021

I'll merge the v5 branch and clean up the issues soon :)

@twiss
Copy link
Member

twiss commented Feb 9, 2021

Fixed by #1186 / #1047 :)

@twiss twiss closed this as completed Feb 9, 2021
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

No branches or pull requests