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

NextJS 9.3 polyfills on IE11 break application #10966

Closed
eddyw opened this issue Mar 11, 2020 · 18 comments · Fixed by #10985
Closed

NextJS 9.3 polyfills on IE11 break application #10966

eddyw opened this issue Mar 11, 2020 · 18 comments · Fixed by #10985

Comments

@eddyw
Copy link

eddyw commented Mar 11, 2020

Bug report

Describe the bug

On NextJS 9.2 I had setup to use corejs@3 for Polyfills. Reading that NextJS 9.3 does auto-polyfill for IE11 when needed, I removed the custom babel config and it "works" sometimes.

I started to get this error in IE11 in some pages:
Screen Shot 2020-03-11 at 7 42 32 PM

To Reproduce

I found out that the global polyfills somehow break some libraries (that maybe come with their own polyfills, I don't know). Also, not sure if NextJS 9.3 uses corejs@3.

One library that is broken (displays above error in screenshot) in NextJS 9.3 is Hubspot lib. Just add this:

      <script
            charSet="utf-8"
            type="text/javascript"
            src="//js.hsforms.net/forms/v2.js"
       />

Either in Head in _app.tsx or anywhere else (any page with or without using next/head)

Expected behavior

It should use corejs@3?
It shouldn't break existing libs added through scripts

Additional Information

This babel config worked in NextJS 9.2 without breaking:

    'next/babel',
    {
      'preset-env': {
        useBuiltIns: 'entry',
        corejs: 3,
      },
    },

System information

  • OS: Windows
  • Browser: IE11
  • Version of Next.js: 9.3

Maybe related

@timneutkens
Copy link
Member

Can you provide a full reproduction? It would help investigate the issue.

@timneutkens timneutkens added the please add a complete reproduction Please add a complete reproduction. label Mar 11, 2020
@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens

yarn create next-app
cd my-app

Then, replace pages/Home.js content with:

import * as React from 'react'

const Home = () => {
  return (
    <div>
      <script
        charSet="utf-8"
        type="text/javascript"
        src="//js.hsforms.net/forms/v2.js"
      />
    </div>
  )
}

export default Home

Run yarn dev
As I mentioned above, it doesn't matter if the script is included in _app.(js|tsx) or in any other page.

As a separate issue, my web app has similar issues with other bundled libraries as well (I think ImmerJS). The error is un-debuggable:
Screen Shot 2020-03-11 at 8 59 57 PM
I'm trying to narrow it down to which lib is causing it. So far, it seems like it's ImmerJS. However, I can't really reproduce this particular one in a clean NextJS app. I'll update if I find it.

@timneutkens
Copy link
Member

Seems like the script is executed before the polyfills, but that should happen with what you had before too.

Btw I accidentally had a look into that script and it seems to be incredible bloated, it ships a full version of React + ReactDOM. You should probably defer loading it to somewhere inside a useEffect call so that you don't block hydration (unrelated to the issue at hand).

@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens I don't think that's the issue 🙈
May I suggest changing this whole thing: https://github.com/zeit/next.js/blob/canary/packages/next-polyfill-nomodule/src/index.js
To just:

import 'core-js/stable' // core-js@3 not @2

// Specialized Packages:
import 'promise-polyfill/src/polyfill'
import 'whatwg-fetch'
import 'url-polyfill'
import assign from 'object-assign'
Object.assign = assign

This solved all issues. Including a Syntax Error in regular expression that I just found and it's linked to this comment #7993 (comment)

Honestly, I don't fully understand the purpose of next-polyfill-nomodule. Babel does this already with config:

    'next/babel',
    {
      'preset-env': {
        useBuiltIns: 'entry',
        corejs: 3,
      },
    },

It'll look for browserList defined in, for example, package.json and load polyfills only for those. We had 0.25%, not dead which includes IE11, so it just worked before.

Regardless, would it be possible to have an option to disable default NextJS polyfills to allow the user add their own? A custom next.config.js to add all stable would look like:

      const originalEntry = config.entry
      config.entry = async () => {
        const entries = await originalEntry()

        if (entries['static/runtime/polyfills.js']) {
          entries['static/runtime/polyfills.js'] = [
            require.resolve('core-js/stable'), // <<<<<<<< use stable core-js@3
            entries['static/runtime/polyfills.js'], // <<<<<<<< cannot remove because NextJS crashes
          ]
        }

        return entries;
      }

However, this defeats the whole purpose of having preset-env with targets because it'll ignore browserList.

@Skona27
Copy link

Skona27 commented Mar 11, 2020

👍 Also experiencing this issue.
After update from 9.2.2 to 9.3.0, my application doesn't work on IE11. Those errors are logged in the console:

ie11

@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens I took some time to make a repo: https://github.com/eddyw/nextjs-9.3-ie11-bug
Run:

yarn dev

See it crash.
Rename _next.config.js to next.config.js (uses core-js@3/stable)
Run:

yarn dev

Works!

@timneutkens
Copy link
Member

timneutkens commented Mar 11, 2020

May I suggest changing this whole thing: /packages/next-polyfill-nomodule/src/index.js@canary
To just:

import 'core-js/stable' // core-js@3 not @2

// Specialized Packages:
import 'promise-polyfill/src/polyfill'
import 'whatwg-fetch'
import 'url-polyfill'
import assign from 'object-assign'
Object.assign = assign

Looking at what /stable actually import it covers more than the polyfills included in next-polyfill-nomodule.

Honestly, I don't fully understand the purpose of next-polyfill-nomodule. Babel does this already with config:

'next/babel',
{
'preset-env': {
useBuiltIns: 'entry',
corejs: 3,
},
},
It'll look for browserList defined in, for example, package.json and load polyfills only for those. We had 0.25%, not dead which includes IE11, so it just worked before.

Babel is only ran over application code, not node_modules, on top of that useBuiltIns causes the polyfills to be imported in the bundles that are sent to modern browsers, which increases bundle size tremendously. Hence why next-polyfill-nomodule only includes the polyfills needed to support IE11 and it only loads those polyfills in browsers that do not support esmodules.

Regardless, would it be possible to have an option to disable default NextJS polyfills to allow the user add their own? A custom next.config.js to add all stable would look like:

No, it should just work out of the box and the initial issue you created should be fixed.

@timneutkens timneutkens added Type: Bug and removed please add a complete reproduction Please add a complete reproduction. labels Mar 11, 2020
@timneutkens
Copy link
Member

Looking at what /stable actually import it covers more than the polyfills included in next-polyfill-nomodule.

To illustrate why we're not going to go with that solution:

Current:
Screen Shot 2020-03-11 at 17 17 10

What you're suggesting:
Screen Shot 2020-03-11 at 17 17 00

@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens alright, off work now.

So I found out that this line causes issues:
https://github.com/zeit/next.js/blob/canary/packages/next-polyfill-nomodule/src/index.js#L64

Replaced with:

import 'whatwg-fetch/fetch'

And fixes one issue.

Another issue I found is again with next-polyfill-nomodule. It doesn't ship with enough polyfills for things such as RegExp[1] and I also don't see typed arrays in there. It's difficult to understand the errors thrown in IE11, so I just wrote a script to generate polyfill-nomodule as you guys have but using core-js@3 and core-js-compat:

This is the script: eddyw/nextjs-9.3-ie11-bug@788e8b2#diff-8ca0d40421065a36dbd988289e8a40f5
This is the generated polyfill-nomodule with target IE 11: eddyw/nextjs-9.3-ie11-bug@788e8b2#diff-f8a56ba1f0d1b020949a8a97fc1555e8

As you can see it's .. bigger than the one NextJS currently uses but includes more polyfills for IE11 that NextJS doesn't include.

Generating the polyfills for IE11 (with core-js@3) is just a couple of lines of code (as linked above to my repo):

const { targets } = require('core-js-compat')({
  targets: 'IE 11',
  filter: /^(es|web)\./,
})

My proposal is, could NextJS (optionally) generate polyfills for the specified targets (polyfill-nomodule.js) if a user passes a browserTargets property (or any better name)? If a user doesn't pass this property, then NextJS could use its minimal version of polyfills (polyfill-nomodule.js). The reason is because you just can't cover all use cases, so IMHO, it's not wise to assume that the minimal polyfills it ships with are enough for all apps.

Note

The current code in my repo replaces the NextJS polyfills with the ones automatically generated by core-js-compat:

        entries['static/runtime/polyfills.js'] = [POLYFILL_NOMODULE]

And it works.

Ref

  1. I think it was es.regexp.sticky not covered among others. I don't have a specific case right now. If you Google a bit, there are plenty of IE11 issues with NextJS regarding RegExp

timneutkens added a commit to timneutkens/next.js that referenced this issue Mar 11, 2020
Also updated to the core-js@3 features modules instead of importing the exact modules directly.

Fixes vercel#10966
@timneutkens
Copy link
Member

timneutkens commented Mar 11, 2020

So we traced the failure down to that hubspot script overriding Promise and setting it to undefined in certain cases, but 'Promise' in window would be true. Replaced the polyfill on our side with the core-js one which is quite a bit larger, but it's not a problem when it's loaded only in older browsers.

It doesn't ship with enough polyfills for things such as RegExp[1] and I also don't see typed arrays in there.

Good catch on the RegExp one, updated it here:
https://github.com/zeit/next.js/pull/10985/files#diff-41195899b53fd88f258c45bb397d00b1R26

Typed arrays are not polyfilled by default as it seemed unlikely they'd be needed for the majority of applications, if you do need them you can load the polyfill only in the location that uses it.

One of the reasons I created a custom list is that core-js-compat results in too many polyfills also, eg it includes these:

import 'core-js/modules/es.string.fixed'
import 'core-js/modules/es.string.fontcolor'
import 'core-js/modules/es.string.fontsize'
import 'core-js/modules/es.string.italics'
import 'core-js/modules/es.string.link'
import 'core-js/modules/es.string.small'
import 'core-js/modules/es.string.strike'
import 'core-js/modules/es.string.sub'
import 'core-js/modules/es.string.sup'

Which no one would use today as they're deprecated and discouraged: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/small

@danlutz
Copy link
Contributor

danlutz commented Mar 11, 2020

@timneutkens : Is it recommended to remove all babel transpiling and plugins like next-transpile-modules with the new 9.3 version?

Don't know if this is the correct place to ask, but we also ran into IE11 problems after upgrading today. Thanks for the enhanced performance! 💯

@timneutkens
Copy link
Member

Is it recommended to remove all babel transpiling and plugins like next-transpile-modules with the new 9.3 version?

Depends on what you're doing exactly, would be best to post a GitHub Discussion instead.

@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens if you want to keep it minimal, then I think NextJS should go carefully through all polyfills generated by core-js-compat when target is IE 11. I agree that some of them are deprecated or discouraged but even the one in PR is missing plenty of ES6 features that change the default behavior of some built-in methods that IE11 may have.

Here is another broken case that I managed to figure out why my app was breaking (Related to ImmerJS breaking):

  • Object.prototype.preventExtensions isn't polyfilled
    Reproduce with:
import * as React from "react"

const Home = () => {
  const [whatIsIt] = React.useState(() => {
    try {
      return Object.preventExtensions(10)
    } catch (e) {
      return e.message.toString()
    }
  })

  return (
    <div>
      <div>It should return number: {whatIsIt}</div>
    </div>
  )
}

export default Home

In Chrome, you'll get:

It should return number: 10

In IE11, you'll get:

It should return number: Object.preventExtensions: argument is not an object

🤷‍♀

Quote from MDN:

In ES5, if the argument to this method is not an object (a primitive), then it will cause a TypeError. In ES2015, a non-object argument will be treated as if it was a non-extensible ordinary object, simply return it.

Using the complete set of polyfills for IE11 works. Or add:

import 'core-js/modules/es.object.prevent-extensions'

To sum up, some IE11 built-ins still need to have polyfills. Same thing for Object.isSealed, Object.isFrozen, Object.isExtensible which have different behavior in ES6.

Then, do I need to create a case for math? 😆 because I'm kind of sure Math.log10 doesn't work either (among all its other newest methods)

So, I'll insist on my proposal 😅 . Could the user specify OPTIONALLY the targets to for core-js-compat to generate the polyfills entry? because the go-to solution for NextJS limitations is always modify the Webpack config but that's not reliable as NextJS new updates may change internals or the entry name, etc etc.

@timneutkens
Copy link
Member

Note that the main goal of the change was to get rid of transform-runtime, which we did, and we added in a bunch of extra polyfills that were commonly needed. The main goal was not to support the exact es6 environment as in many cases eg the Math ones are never used and you should add the polyfill in the place where it's used to ensure it's only loaded when the code is needed.

That is to say, I'm not opposed to adding in more polyfills if there's a real need fo them like the Object. ones you referred to, we just started with the transform-runtime ones.

@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@timneutkens alright, I guess that makes sense 😅

I just assumed it meant that it'll provide a compatible environment out-of-the-box for IE11 (does it count Safari? 😅) based on what the blog post about 9.3 mentioned:

As part of this compatibility, we also compile your application to be IE11 compatible: this allows you to safely use ES6+ syntax features, Async/Await, Object Rest/Spread Properties, and more—all with zero configuration necessary.

Maybe it'll be good to clarify, somewhere, what this actually .. means? 🤔

@developit
Copy link
Contributor

Just adding my 2¢ here: it's always going to be easiest to argue in favor of polyfilling the world. Even if all of core-js were to be included, that wouldn't fix IE11's DOM bugs or any number of other oddities. Essentially, the polyfilling threshold is always going to be arbitrary, and it's best to recognize that and treat it like the tradeoff that it is.

Ideally, it would be great to have the baseline nomodule polyfill cover some known set of widely used polyfills that bring IE11 roughly up to par with the modern browsers. Then automate contextual polyfilling similar to preset-env, but with the knowledge of what APIs can already be assumed available based on the nomodule polyfill / modern browser baseline. Unfortunately, there's no tools that currently make this possible.

One other thing to note here: it may not be sufficient to just add these polyfills into the current polyfills chunk, since the bugs they fix may be present in browsers that support <script type=module>. I would guess many of the Math.* methods fall into this category, whereas the Object methods only need to be shipped in the nomodule polyfill.

Timer added a commit that referenced this issue Mar 11, 2020
* Use core-js promise polyfill for nomodule browsers

Also updated to the core-js@3 features modules instead of importing the exact modules directly.

Fixes #10966

* Simplify reflect and regexp

* Add ie11 test for bad Promise

* Add test script for regexp and ie11

Co-authored-by: JJ Kasper <[email protected]>
Co-authored-by: Joe Haddad <[email protected]>
@eddyw
Copy link
Author

eddyw commented Mar 11, 2020

@Timer @timneutkens I see PR was merged (and closes this issue). Not to open another thread but test/integration/production/public/regexp-test.js seems to have ... the hubspot script 🤔 . The Hubspot script had only issues with Promise but I didn't see any issue with RegExp that needs polyfiling.

A quickly wrote a case where it failed before PR (I mentioned es.regexp.sticky):

import * as React from "react";

const str1 = "table football";
const regex1 = new RegExp("foo", "y") // sticky
regex1.lastIndex = 6

const Home = () => {
  const isSticky = regex1.sticky // Expected: true
  const isMatch1 = regex1.test(str1) // Expected: true
  const isMatch2 = regex1.test(str1) // Expected: false

  return (
    <div>
      Works!!!
      <div>isSticky: {isSticky.toString()}</div>
      <div>isMatch1: {isMatch1.toString()}</div>
      <div>isMatch2: {isMatch2.toString()}</div>
    </div>
  )
}

export default Home

In IE11:

Syntax in regular expression

@developit

Then automate contextual polyfilling similar to preset-env, but with the knowledge of what APIs can already be assumed available based on the nomodule polyfill / modern browser baseline.

It's what I was suggesting. preset-env actually does use core-js-compat and a custom filter method to include/exclude polyfills based on browsersList query (in targets prop). It's more or less just:

const { targets } = require('core-js-compat')({
  targets: 'IE 11',  // <<< browser baseline. Browserslist query
  filter: /^(es|web)\./, // <<< filter only es and web polyfills
})
Object
  .keys(targets)
  .filter(moduleName => ...) // exclude deprecated, not needed such as es.string.fontcolor

Then you end up with a more or less nomodule-polyfill (excluding whatwg-fetch)

We want to minimize the number of core-js polyfills included to support mainly IE11, Edge, and Safari. It'd be great if we could just do:

// next.config.js
module.exports = {
  browsers: '> 0.25%, not dead'
}

And have core-js-compat generate it for us.

ScriptedAlchemy pushed a commit to module-federation/next.js that referenced this issue Mar 17, 2020
* Use core-js promise polyfill for nomodule browsers

Also updated to the core-js@3 features modules instead of importing the exact modules directly.

Fixes vercel#10966

* Simplify reflect and regexp

* Add ie11 test for bad Promise

* Add test script for regexp and ie11

Co-authored-by: JJ Kasper <[email protected]>
Co-authored-by: Joe Haddad <[email protected]>
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants