WICG / inert

Polyfill for the inert attribute and property.
Other
924 stars 81 forks source link

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

Closed timbomckay closed 5 years ago

timbomckay commented 5 years ago

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 commented 5 years ago

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

robdodson commented 5 years ago

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 commented 5 years ago

cc @aomarks

timbomckay commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

timbomckay commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Ping on the major release that's not transpiled?

robdodson commented 5 years ago

Just released 3.0

aomarks commented 5 years ago

Thank you Rob!