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

Prepare package exports for webpack 5 #468

Merged
merged 7 commits into from
Jun 22, 2020

Conversation

ctavan
Copy link
Member

@ctavan ctavan commented Jun 18, 2020

Closes #462

@ctavan
Copy link
Member Author

ctavan commented Jun 18, 2020

@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 webpack@5.0.0-beta.18 and from what I can tell it produces the desired bundles.

@ctavan ctavan requested review from broofa, TrySound and LinusU June 18, 2020 07:21
Copy link

@sokra sokra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm.

It could be discussed if the default should be a node version or a browser version.

@ctavan
Copy link
Member Author

ctavan commented Jun 18, 2020

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?

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I'm going to abstain since I'm kind of losing track of the requirements for this module's build system. (Not necessarily a bad thing, btw.)

image

package.json Outdated
"require": "./dist/index.js",
"import": "./wrapper.mjs"
}
"browser": "./dist/esm-browser/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • electron supports the Node.js API
  • worker supports (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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, thanks for helping drive these discussions

This was referenced Oct 27, 2024
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.

4 participants