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

remove asn1 from external deps #1163

Closed
wants to merge 1 commit into from
Closed

Conversation

vordimous
Copy link

this could resolve #925,

but does resolve this issue

@twiss
Copy link
Member

twiss commented Sep 28, 2020

Hi 👋 Thanks for the PR.

So - a few points:

  • asn1.js is only required in Node.js, not in the browser. Therefore, in OpenPGP.js v4, I would prefer not to add it to the bundle.
  • However, in OpenPGP.js v5 (V5 #1047), we will have a separate bundle for Node.js and for the browser. So there, I would be more open to adding it to the Node.js bundle.
  • Even there, however, it would seem to me (but I might be wrong) that the same problem exists for the other dependencies of OpenPGP.js, it's just that they are less noticable. For example, if you call new openpgp.Keyring(), I guess it would throw since the node-localstorage dependency is missing?
  • If that's the case, it would seem a bit suboptimal to me to include all the dependencies in the build, since some of them are sometimes not used.
  • Since these dependencies are clearly listed in package.json, it seems to me that there's some other issue, either in the build process of openpgp.js, or ghaction-import-gpg, or in ncc, and I feel like this should be fixed differently.
  • If you want, you could check whether this issue has been resolved in OpenPGP.js v5, by pointing package.json to "openpgp": "github:openpgpjs/openpgpjs#74f16f080c9b5a3c9b0a51ffdd869a7d07a797fe". (There are however also breaking API changes in v5, see https://github.com/openpgpjs/openpgpjs/wiki/V5-Changelog#Examples.)

@vordimous
Copy link
Author

It makes sense this conflict would exist when trying to build for node and browser support. I will try to point to V5 and hope that works.

As far as ncc goes, I would think that the import strategy in general needs to be specific enough for it to know what libs in needs to compile into the single binary. I did try to alter the code a bit to see if that is what was causing ncc not to see "asn1.js" as a dep to include.

I will report back after trying V5.

@vordimous
Copy link
Author

Ok @twiss, I can confirm that V5 works to resolve the dependency issue. I was able to package and deploy the ncc compiled node script without having any module references missing.

Do you have a timeline for V5, specifically when the typscript type definitions will be available, or at least a .d.ts file?

@twiss
Copy link
Member

twiss commented Sep 29, 2020

Thanks for checking!

We don't maintain the TypeScript bindings. The types at @types/openpgp are maintained by external contributors; see #561. We would be open to a PR to the v5 branch that adds types, however.

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.

Error: Cannot find module 'asn1.js' Error: Cannot find module 'openpgp'
2 participants