Conversation
|
@sokra would you mind reviewing this? Does this match your recommendations for webpack (referenced in webpack/webpack.js.org#3785)? I've tested it with |
sokra
left a comment
There was a problem hiding this comment.
Lgtm.
It could be discussed if the default should be a node version or a browser version.
Or should be left out as you initially suggest in your gist, but what was criticized by @guybedford in https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62#gistcomment-3340001. @guybedford isn't this a good example of where a default just doesn't really offer good future-proof contract? |
package.json
Outdated
| "require": "./dist/index.js", | ||
| "import": "./wrapper.mjs" | ||
| } | ||
| "browser": "./dist/esm-browser/index.js", |
There was a problem hiding this comment.
I'd suggest making this "default" and moving it to the end as per our current discussions.
This way we don't have to assume that "browser" and "node" are the only possible conditions matched.
Although @sokra may well have different advice :)
There was a problem hiding this comment.
I find it really hard to decide for a default because that means anticipating whether any future platforms beyond node and browser would rather be like Node.js (and hence support the Node.js crypto API) or rather like the browser (and hence support the web Crypto API)…
Just as an example from what is already supported by webpack:
electronsupports the Node.js APIworkersupports (mostly) WebCrypto
So there just doesn't seem to be a reasonable default 🤷♂️
@sokra, do I understand correctly that I will have to specify separate keys for electron and worker to make this work with webpack@5?
There was a problem hiding this comment.
@ctavan I would generally advise a browser-like environment with global checks as the universal baseline over assuming Node.js APIs - which is why it can make sense to have a Node.js-specific build since that is the one that makes import assumptions for the availability of a core crypto library. Electron and Worker would still match the node condition I believe in Webpack.
For example, if a new JS environment were to be released that supported a custom "core-api" import, then you would also want to branch specifically into that environment to support import 'core-api'.
These use cases will also be improved with support for "imports" allowing branching at the import level rather than the export level.
There was a problem hiding this comment.
Yes I would also advise to use browser as default. Electron always comes with node or browser specified too (there are multiple environments in electron).
There was a problem hiding this comment.
@sokra does that mean I have to specify electron explicitly or will webpack pick either node or browser depending on the respective electron environment? (If so, this should be clarified in your webpack documentation draft since I couldn’t get that info from there)
There was a problem hiding this comment.
You don't have to mention electron. Browser and node condition will also cover most electron use cases.
You should only use electron specifically when using electron specific code.
guybedford
left a comment
There was a problem hiding this comment.
Good stuff, thanks for helping drive these discussions
Closes #462