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

Microsoft Edge 16-18 missing "flat" polyfill #11173

Closed
dawsbot opened this issue Mar 18, 2020 · 12 comments · Fixed by #17083
Closed

Microsoft Edge 16-18 missing "flat" polyfill #11173

dawsbot opened this issue Mar 18, 2020 · 12 comments · Fixed by #17083
Assignees
Milestone

Comments

@dawsbot
Copy link

dawsbot commented Mar 18, 2020

Bug report

Describe the bug

When loading out Next site with Edge 18, there is a critical TypeError stating Object doesn't support property or method 'flat'

To Reproduce

  1. Go to https://epdev123.org/wiki/lang_en/magnus-dunn in Edge 18
  2. Notice the full-screen critical error

Expected behavior

This page should load properly like all other browsers were able to

Screenshots

image

System information

  • OS: Windows
  • Browser: Edge 18, Edge 17, Edge 16 (see Sentry Issue for browser breakdown)
  • Version of Next.js: 9.3.1

Additional context

This issue may relate to the recent changes in Next's polyfilling partially described in #7993

  • I looked into adding this flat polyfill manually, but it's already included in next-polyfill-nomodule as import 'core-js/features/array/flat'

  • We have zero polyfilling in our application currently, we're fully relying on Next's included polyfills. We have not changed the included version of core-js

  • This is a private project, we can email a zip to anyone at Zeit, just let me know if this looks like a valid bug to you all

@dawsbot dawsbot changed the title Edge 18 missing "flat" polyfill Microsoft Edge 16-18 missing "flat" polyfill Mar 18, 2020
@dawsbot
Copy link
Author

dawsbot commented Mar 18, 2020

This was "solved" just a few days ago via 20c5467, but still is not working for us.

Initially, we saw this error on next 9.3.0. We upgraded our deployed next version to 9.3.1-canary.4 and still see this issue. Then we upgraded to 9.3.1 and still see this issue.

@alexanbj
Copy link

I've seen the same for our site. The problem is that Edge 16-18 doesn't support flat(Map), but has support for nomodule, meaning that the polyfills won't be downloaded.

@Timer Timer added this to the 9.3.x milestone Apr 15, 2020
@Timer Timer self-assigned this Jul 10, 2020
@martpie
Copy link
Contributor

martpie commented Sep 14, 2020

So is there an actual solution to the issue? rather than importing all the polyfills manually? (or just staying on a previous version of Next.js?)

@Timer
Copy link
Member

Timer commented Sep 14, 2020

@martpie I've removed your comment because it causes severe performance regressions and should not be used under any circumstances. If you'd like to polyfill these manually, you should only import a flat polyfill, not an entire ES6+ environment polyfill.

@vercel vercel deleted a comment from martpie Sep 14, 2020
@martpie
Copy link
Contributor

martpie commented Sep 14, 2020

@Timer I understand the motivation, but I disagree with it so I will just share my thoughts:

Next.js has this built-in polyfilling system that the developer has no control over. Right now, this system is broken on one of the supported platform, so any application making use of ES6 feature will be broken on Edge, and this is a critical issue in my case.

Now, the important bit: for me, the perfs cost of a one-line fix was less important than wasting time being sure I import only the polyfills I use through my code: my devs work in ES6 and they should not have to think every time they use a ES6 feature "oh do I have to include a polyfill for this method?". It's too much time wasted. Especially when you have an existing codebase: it makes no sense I read all my files and check all the ES features I use to be sure I only import the polyfills I should use.

Hence the one-liner.

it causes severe performance regressions

Yes, it adds a 20 kilobytes, the comment was explicit about it, that's why I thought it was an ok solution.

Basically, bad solution over no solution, but if you believe otherwise, this is fine.

@Timer
Copy link
Member

Timer commented Sep 14, 2020

It adds over 20kb gzipped:

lerna info run Ran npm script 'prepublish' in '@next/polyfill-nomodule' in 26.5s:
yarn run v1.22.4
$ microbundle src/index.js -f iife --no-sourcemap --external none
Build "polyfillNomodule" to dist:
      30.9 kB: polyfill-nomodule.js.gz
      27.8 kB: polyfill-nomodule.js.br

This is a ton of JavaScript which prevents your app from bootstrapping. If you're OK with the performance degradation in your app, that's fine.

Your example also used the pattern from with-polyfills which has a warning that it can prevent your application from hydrating.


Now, the important bit: for me, the perfs cost of a one-line fix was less important than wasting time being sure I import only the polyfills I use through my code: my devs work in ES6 and they should not have to think every time they use a ES6 feature "oh do I have to include a polyfill for this method?". It's too much time wasted.

It'd be great if you could send a PR to Next.js that fixes this specific missing polyfill.

@martpie
Copy link
Contributor

martpie commented Sep 14, 2020

as @alexanbj said:

I've seen the same for our site. The problem is that Edge 16-18 doesn't support flat(Map), but has support for nomodule, meaning that the polyfills won't be downloaded.

it's not about one missing polyfill, it's about all the polyfills missing on Edge 😄(If I understood the bug correctly). I may have time to submit a PR this week, but it feels much bigger than just "add an import for a missing polyfill". Or I misunderstood something.

@Timer
Copy link
Member

Timer commented Sep 14, 2020

There's a small uncanny valley of features that are missing in Edge versions that support nomodule. We only need to polyfill those specific features.

@martpie
Copy link
Contributor

martpie commented Sep 14, 2020

Thanks for the tip 👍 I'll have a look and open a PR this week

@Timer
Copy link
Member

Timer commented Sep 14, 2020

I opened something here:
#17083

@Timer Timer added the point: 2 label Sep 14, 2020
@Timer Timer modified the milestones: 9.x.x, iteration 9 Sep 14, 2020
@Timer
Copy link
Member

Timer commented Sep 15, 2020

This is fixed in the latest Next.js canary version.

@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 29, 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.

5 participants