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

Latest release (module: "src/inert") broke our builds #136

Closed
timbomckay opened this issue Sep 9, 2019 · 11 comments
Closed

Latest release (module: "src/inert") broke our builds #136

timbomckay opened this issue Sep 9, 2019 · 11 comments

Comments

@timbomckay
Copy link
Contributor

#135 Broke our builds because it changed which file our webpack pulled in. This probably should've been a breaking change as it took us 2 days to track down why our builds were breaking.

You should put this in the documentation so people using this in webpack know what to do.

@bcutler-work
Copy link

This also appears to have broken our bundles on IE11 because the source was no longer being passed through Babel.

@robdodson
Copy link
Collaborator

ugh... I didn't realize webpack had this "feature". I didn't think it was a breaking behavior because my assumption was that most folks were using the version that main pointed at. We added the module version after a different team requested a version that wasn't transpiled to UMD.

In my opinion, it feels a bit weird to ship a version that uses module syntax but transpiles everything else to ES5...

As a short term fix, y'all could pin the dependency to the previous version or try the mainFields webpack option mentioned here. But I realize mainFields is annoying because it affects more than just this one dependency. Let me follow up with some peers to see how they're shipping non-transpiled and transpiled code in the same packages to see if I can come up with a solution.

@robdodson
Copy link
Collaborator

cc @aomarks

@timbomckay
Copy link
Contributor Author

Yeah we created some webpack aliases when we discovered this. You weren't the only package to release this feature last week. Feel like this should've been a webpack v5 feature but maybe I don't quite understand it enough. Might be meant for server side rendering instead of client-side?

bowser$: 'bowser/bundled',
'wicg-inert$': 'wicg-inert/dist/inert'

Thanks for the mainFields alias suggestion. Perhaps it could prevent future breaks from other packages. Seems like this is going to continue to popup.

@robdodson
Copy link
Collaborator

So because this broke folks I was thinking of shipping a new minor version that removes the module field and basically puts everything back to the way it was. Then shipping a v3 that has the module field.

@timbomckay @bcutler-work @aomarks what do y'all think?

@robdodson
Copy link
Collaborator

twitter discussion for context: https://twitter.com/rob_dodson/status/1172302699204755458

@timbomckay
Copy link
Contributor Author

Yeah this topic has been a discussion for our team recently. Some in the camp of packages should ship ES5 and excluding node_modules from babel, while others are in the camp of consumer's responsible for compiling & polyfilling. Similar to what Rich Harris stated in his tweet.

ship modern ES (generally ESM + CJS/UMD, depending on intended use), and if people want to pay the IE tax that's on them

Alternatively, the mainFields link you provided earlier mentions the browser field. Just now finding this but perhaps that's where compiled code could be provided? I don't know.

In regards to reverting and releasing a new major, I don't think it really matters to us. I don't think we're going to remove our alias unless it's in favor of the mainFields option. Just have clear documentation, maybe a warning/alert at the top for an amount of time to help people visiting the repo in search for an answer. Maybe if there's a way to add a postinstall message to make it known? Or perhaps reverting and providing the message as a warning for the next major? I don't know, I'm not sure what's best practice when the damage has been done.

You also released this as a minor/feature update, which makes sense and we've discussed changing our dependencies from ^ to ~ to only take patches instead of feature changes for cases such as this. It looks like webpack actually released the module & browser support a long time ago, possibly from the initial release of v4. Perhaps you're just doing things correctly now while our webpack config needed to be target: 'web' or mainFields: 'browser'. I do think it's weird that webpack defaults to module over main instead of explicitly defining module as the preferred field, but I'm sure that included a lengthy discussion in the community.

So I don't know, just try to document/inform wherever possible.

@robdodson
Copy link
Collaborator

Just released a patch version which updates the module field to point at a transpiled version. I'll do a follow up major release tomorrow.

@aomarks
Copy link
Contributor

aomarks commented Sep 23, 2019

Ping on the major release that's not transpiled?

@robdodson
Copy link
Collaborator

Just released 3.0

@aomarks
Copy link
Contributor

aomarks commented Sep 23, 2019

Thank you Rob!

aomarks added a commit to material-components/material-web that referenced this issue Sep 24, 2019
This version has a "module" field that resolves to ES2015 code (uses
native ES2015 classes instead of transpiled). See
WICG/inert#136 for more context.
aomarks added a commit to material-components/material-web that referenced this issue Sep 24, 2019
This version has a "module" field that resolves to ES2015 code (uses
native ES2015 classes instead of transpiled). See
WICG/inert#136 for more context.
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

No branches or pull requests

4 participants