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
Basic Deno support #1448
Basic Deno support #1448
Conversation
7def5e5
to
3877f7b
Compare
All tests passing now, force pushed some fixes as no one seem to have looked at it yet. |
3877f7b
to
037a641
Compare
Another force push fixing linting |
|
Oh! Got an error (in Deno) saying navigator isn't defined, so I made an assumption that everything following navigator is undefined. Need to have a closer look at what was going on. |
037a641
to
1874aca
Compare
Slightly simplified after feedback from @separaterecords, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello 👋 Thanks for the PR! Some small comments below.
README.md
Outdated
Import as an ES6 module, from an .mjs file on a CDN of your choice. | ||
|
||
```js | ||
import * as openpgp from "https://cdn.jsdelivr.net/npm/openpgp@5/dist/openpgp.mjs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a security-related module like OpenPGP.js, I would prefer not to suggest to use a CDN, at least without something like Subresource Integrity (don't know if it's possible to add it here?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe deno.land/x can be seen as a more trusted source? This will require a maintaner to register the module at https://deno.land/x
, and add a webhook to github.
For now, maybe we can keep it anonymous, with an example showing a local import?
Edit: Suggested a more anonymous approach in latest commit, see new single comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, yeah. We can do that after merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet 👍
Hi, I am not sure if this is related as I am not using Deno, but I see that one of the problems in this issue was references to My question on StackOverflow has more details. I have imports like this: import { encrypt, readKey, createMessage, decryptKey, readPrivateKey } from 'openpgp/lightweight'; And my npm dependency is on this merged branch, so I expected it would work: "node_modules/openpgp": {
"version": "5.1.0",
"resolved": "git+ssh://git@github.com/openpgpjs/openpgpjs.git#bd13edfc884cf6b6b7f715099529ac6a656733c6",
"license": "LGPL-3.0+",
"dependencies": {
"asn1.js": "^5.0.0"
},
"engines": {
"node": ">= 8.0.0"
}
}, The error I get when I run
This happens with If you think this is unrelated, I can open a new issue. |
@renatoathaydes Yeah, please create a separate issue or PR for this :) |
Related to Discussion #1447
Changes made:
src/*
, to avoid directly accessing objects/properties not defined in a Deno environment, specificallynavigator.userAgent
andnavigator.hardwareCurrency
Tested with:
npm test
npm run browsertest
npm run lint
Testable by:
npm run build
to generatedist/openpgp.mjs
test.js
- (in this example in a directory adjacent to openpgpjs../denotest
)deno run test.js