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

Breaking change happened on v8.3.0 (IE 11 support lost) #551

Closed
jasonwilliams opened this issue Jan 25, 2021 · 8 comments
Closed

Breaking change happened on v8.3.0 (IE 11 support lost) #551

jasonwilliams opened this issue Jan 25, 2021 · 8 comments
Labels
more info needed Issue not actionable w/out additional details

Comments

@jasonwilliams
Copy link

jasonwilliams commented Jan 25, 2021

This package claims to support IE 11 on the readme. But that stopped working on a minor release.
From what I can gather, v8.3.0 introduced stringify.js which utilises default parameters. IE 11 does not support default parameters and thus fails.

I've managed to fix this issue by using babel, but its worth:

  • Not using modern syntax in minor releases until checking they work in all browsers you're supporting
  • Either remove IE 11 support or remove usage of default params
  • Using some rollup plugin to compile down to a compatible version

Related to:
#469

@ctavan
Copy link
Member

ctavan commented Jan 25, 2021

@jasonwilliams thank you for reporting. IE11 is still officially supported.

It sounds like you might be running into the same issue as #516 ? It looks like your bundler is picking up the Node.js build instead of the browser build. Let me know if the suggestions in the other issue work for you as well.

@ctavan ctavan added the more info needed Issue not actionable w/out additional details label Jan 25, 2021
@jasonwilliams
Copy link
Author

jasonwilliams commented Jan 25, 2021

my mainfields is set to config.resolve.mainFields = ['browser', 'main', 'module']; which i believe is correct.

@ctavan
Copy link
Member

ctavan commented Jan 25, 2021

Well, this explains what you're seeing at least, webpack is picking up the Node.js build: #516 (comment)

I believe 'main' and 'module' should be swapped as you almost always want to make use of the benefits of ESModules when bundling for the browser (most importantly: tree shaking).

@jasonwilliams
Copy link
Author

jasonwilliams commented Jan 25, 2021

I believe 'main' and 'module' should be swapped as you almost always want to make use of the benefits of ESModules when bundling for the browser (most importantly: tree shaking).

That was intentional due to an unrelated issue to this. We had some issues with a package in their "esm" build but their "cjs" build was fine so we needed to force webpack to prefer that version.

As long as "browser" is first why would those two being swapped matter?

@ctavan
Copy link
Member

ctavan commented Jan 25, 2021

In the browser config we can only specify file overrides but not entry points. In order to support all the various platforms that we want to support, the different fields in package.json are crafted the way the are:

  • pkg.main is picked up when using the module directly in older Node.js versions. This is the Node.js CommonJS build.
  • pkg.module was originally introduced by webpack for being able to ship an ESM build for bundling. In order to allow webpack builds for Node.js, this points to the Node.js ESModule build.
  • pkg.browser now defines overrides when bundling for the browser but assumes that browser builds want to use the ESM build. We don't include a CommonJS build for the browser as we haven't really come across a use case for that.

So in the specific scenario where you define config.resolve.mainFields = ['browser', 'main', 'module'];, the 'main' and 'module' fields inform webpack about the entrypoint to be used. Since 'main' comes first, webpack will pick the Node.js CommonJS build, and won't find a browser-specific override.

If you swap the order, then webpack will pick the Node.js ESModule build and subsequently find the corresponding browser-specific overrides and use the Browser-specific ESModule build instead.

So the use case where you instruct webpack to use CommonJS builds instead of ESModule builds is one which we currently do not support.

BTW once you upgrade to webpack@5 this problem will also vanish because webpack@5 uses the definitions from pkg.exports instead.

We could "fix" this by providing a CommonJS Browser build. We have deliberately not done this so far because it would further increase the package size while we are not convinced about any clear benefit this would add.

@jasonwilliams
Copy link
Author

jasonwilliams commented Jan 25, 2021

For now i've locked to an older version until I can update Webpack to v5.
Thanks for your detailed reply.

Something like that may be worth adding to the documentation, as im sure for various reasons others may have switched the ordering.

@compulim
Copy link

compulim commented Feb 12, 2021

How about adding an additional browser override like? Along with updated build pipeline to emit stringify-browser.js.

  "browser": {
    "./dist/md5.js": "./dist/md5-browser.js",
    "./dist/rng.js": "./dist/rng-browser.js",
    "./dist/sha1.js": "./dist/sha1-browser.js",
+   "./dist/stringify.js": "./dist/stringify-browser.js",
    "./dist/esm-node/index.js": "./dist/esm-browser/index.js"
  },

FYI, the cause was IE11 does not support default parameters:

- function stringify(arr, offset = 0) {
+ function stringify(arr, offset) {
+   if (typeof offset === 'undefined') {
+     offset = 0;
+   }

    // Note: Be careful editing this code!  It's been tuned for performance
For people who are blocked on IE11, if you are using Webpack 4 and has access to webpack.config.js, you can add a rule to transpile uuid/dist/stringify.js. It is a stopgap solution and should be taken with care. As mentioned by @ctavan, bumping to Webpack 5 will resolve the issue too.
  module: {
    rules: [
      {
        test: /\/node_modules\/uuid\/dist\/stringify\.js$/,
        use: {
          loader: 'babel-loader',
          options: {
            presets: [
              [
                '@babel/preset-env',
                {
                  forceAllTransforms: true,
                  modules: 'commonjs'
                }
              ]
            ]
          }
        }
      }
    ]
  },

@ctavan
Copy link
Member

ctavan commented Dec 2, 2021

Closing this for now as:

  1. This is due to a bad configuration of webpack that can and should be fixed in the webpack config (e.g. Remove mainFields in webpack.config.js microsoft/BotFramework-WebChat#3726).
  2. We'll drop IE 11 support officially with the next major release (9.0.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more info needed Issue not actionable w/out additional details
Projects
None yet
Development

No branches or pull requests

3 participants