-
-
Notifications
You must be signed in to change notification settings - Fork 902
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
fix: provide browser versions independent from module system #380
Conversation
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.
@TrySound thank you for working on this, in general the proposed changes look
Using webpack/rollup to bundle for Node.js was indeed not considered yet from our part.
In fact we did use the browser
field in package.json
in the previous version of UUID and it looks like it is the only way to solve the issues around #378.
It would be great if you could amend the commit message and add references to the documentation of webpack and rollup where the precise usage of the browser
fields is documented, I believe that would be:
scripts/build.sh
Outdated
@@ -14,21 +14,12 @@ mkdir -p "$DIR" | |||
# Transpile CommonJS versions of files | |||
babel --env-name commonjs src --source-root src --out-dir "$DIR" --copy-files --quiet | |||
|
|||
# Transpile ESM versions of files for the browser | |||
babel --env-name esm src --source-root src --out-dir "$DIR/esm-browser" --copy-files --quiet | |||
# Transpile ESM versions of files |
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.
Transpile ESM versions of files for use with module bundlers
as we do not yet support native Node.js esmodules.
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.
We should support bundlers with node target.
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.
The point that I wanted to make is that this is still designated for module bundlers and not https://nodejs.org/api/esm.html
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.
So: I agree we should not restrict this to browser but the esm build here is still targeted at module bundlers!
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.
Yes, though commonjs browser stuff can be used by browserify unless it supports modules too.
Also, please rebase as master has changed meanwhile. |
3d2edbc
to
455604e
Compare
@ctavan Added links to commit message. Tests look unstable, not related to my changes. |
@TrySound the code looks good, the build errors are due to lacking Browserstack credentials from your fork. I still have to figure out how to proceed with that. |
455604e
to
2b98aad
Compare
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.
@TrySound I tried out your changes locally and while they seem fine for use with module bundlers they unfortunately break the native browser esmodule support that is showcased in https://github.com/uuidjs/uuid/blob/master/examples/browser-esmodules/example.js
I fear we may have to go back to two builds:
./dist/esm-node/
./dist/esm-browser/
in order to support esmodules natively in both the browser and (eventually) node without bundlers. What do you think?
You mean changed paths is breaking change? |
Ah, got it. Browsers do not get right paths. |
Ok, I can do it. |
No: The thing is that so far we only had one esm build designated at the browser where the browser-specific files have been renamed to just replace the node-specific files. That way, you could use the ESM build directly in the browser. With your change, you rely on the So unfortunately your change breaks the opportunity to use esmodules directly in the browser as shown in the example: https://github.com/uuidjs/uuid/blob/master/examples/browser-esmodules/ Since we don't have the nice workaround of relying on the |
Probably will fix uuidjs#378 Currently main field is used for node and module for browser build. This is not entierly correct. Webpack can be used to build node apps. Though module is always preferred over main. For browser versions there is a special field. In this diff I added esm support for both node and browser. rollup-plugin-node-resolve look at browser field as well and prefer it to bundle umd. https://webpack.js.org/configuration/resolve/#resolvemainfields https://github.com/rollup/plugins/tree/master/packages/node-resolve#browser https://github.com/defunctzombie/package-browser-field-spec
2b98aad
to
0d18874
Compare
@ctavan Looks good now? |
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.
Looks good now! Will pull this in and add some minor nits on top.
Probably will fix #378
Currently main field is used for node and module for browser build. This
is not entierly correct. Webpack can be used to build node apps. Though
module is always preferred over main. For browser versions there is a
special field.
In this diff I added esm support for both node and browser.
rollup-plugin-node-resolve look at browser field as well and prefer it
to bundle umd.