vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.78k stars 26.95k forks source link

Next.js issue starting in 9.2.0 #10258

Closed jasontaylor137 closed 4 years ago

jasontaylor137 commented 4 years ago

Bug report

Describe the bug

I have a next.js app which uses exclusively static rendering. I recently upgraded it to 9.2.0 and released it to production. Everything worked.

After several more commits, we started to see our script just not start executing. That's how it looks. No console errors, no network errors, but none of the script seems to run. We have ajax loading spinners that are baked into the static html of the site by the build process, and that's all we see. The ajax loads never happen. Apparently we are getting no script running.

"yarn dev" / localhost:3000 works fine. It's only when building production.

So I have a specific commit based on 9.2.0 that works, and a subsequent one that does not. However, nothing about the commit where it broke has anything I would ever suspect to be related to this. And if I go to our latest commit and revert the one where the break starts, it is still broken.

It's as if it has more to do with the quantity of code than what the code is??

Taking all our changes but reverting to 9.1.7 resolves the issue. I tested 9.2.1 and it does NOT resolve the issue.

To Reproduce

My challenge is how to give you a reproducible test case. I can't share my code obviously and I don't know how to boil this down to something minimal quite yet.

Expected behavior

I'd like the benefits of 9.2.x and have my script keep working.

Screenshots

Not sure these would help

System information

Additional context

jmswrnr commented 4 years ago

Sounds very similar to an issue I encountered, where code would be chunked separately and marked as a dependency in Webpack, but the chunk isn't added as a script tag on the page, preventing the main JavaScript entry point from executing. By any chance are you adding code to main.js using next.config.js ? #10154

jasontaylor137 commented 4 years ago

Adding code to main.js - guilty as charged. I have this at the bottom of my next.config.js:

  /* ugh polyfills, see https://github.com/zeit/next.js/issues/2060#issuecomment-385199026 
  we need to keep this and the changes in .babelrc and babel-polyfill until IE support 
  completely goes away, otherwise next.js is so broken it can't even display the warning 
  that IE doesn't work */

  // Unshift polyfills in main entrypoint.
  const originalEntry = config.entry;
  config.entry = async () => {
     const entries = await originalEntry();
     if (entries['main.js']) {
        entries['main.js'].unshift('./polyfills.js');
     }
     return entries;
  };
Timer commented 4 years ago

Closing as duplicate of https://github.com/zeit/next.js/issues/10154

alexparish commented 4 years ago

Given this approach is no longer valid, what is the best practice for importing a polyfills.js file? Would you import it directly from _document.js?

jasontaylor137 commented 4 years ago

I believe they are going to fix the fundamental issue. I'm stuck on 9.1.7 until they do. Have a look at the linked item which is a reproducible test case.

alexparish commented 4 years ago

I believe they are going to fix the fundamental issue. I'm stuck on 9.1.7 until they do. Have a look at the linked item which is a reproducible test case.

Thanks, that wasn't immediately obvious to me.

I've managed to upgrade to 9.2.1 by importing my polyfills.js file at the top of _app.js:

// Replace with polyfill.io or script nomodule tag
// so we're not sending the polyfills to modern browsers
import '../polyfills';

import React from 'react';
import NextApp from 'next/app';
import NextHead from 'next/head';
jasontaylor137 commented 4 years ago

Very nice. Because of the way this issue impacted me, I'd be a bit nervous going that way, because it may work now and then stop working when your bundle sizes change enough that things are chunked differently. But perhaps hooking it into _app.js makes you immune to this.

alexparish commented 4 years ago

Very nice. Because of the way this issue impacted me, I'd be a bit nervous going that way, because it may work now and then stop working when your bundle sizes change enough that things are chunked differently. But perhaps hooking it into _app.js makes you immune to this.

Don't suppose you could advise @timneutkens ?

timneutkens commented 4 years ago

@alexparish what do you have in the polyfills file?

alexparish commented 4 years ago

@timneutkens

// TODO: 39kb saving (gzipped) if removed.
require('core-js/stable');
require('regenerator-runtime/runtime');
require('intl');
alexparish commented 4 years ago

I've just tested this approach - importing the polyfills.js file at the top of _app.js - and it's not working.

I'm using Array.prototype.includes and am seeing errors in Chrome 45 where this function is not available.

timneutkens commented 4 years ago

Can you add this to your next.config.js and completely remove that polyfills chunk. I've been working on shipping all the polyfills needed for IE11 only to older browsers automatically.

module.exports = {
  experimental: {
    polyfillsOptimization: true
  }
}

Intl is supported in IE11+ for the most part: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl

alexparish commented 4 years ago

@timneutkens That is very cool. Have given it a go in IE11 and it polyfilled most of what I needed, but not Array.prototype.includes. Is there a list of what features it polyfills?

timneutkens commented 4 years ago

It's included in the list: https://github.com/zeit/next.js/blob/canary/packages/next-polyfill-nomodule/src/index.js#L50

Can you provide a reproduction.

alexparish commented 4 years ago

It's working now I've upgraded to the canary version. Thanks.

alexparish commented 4 years ago

@timneutkens Is regenerator-runtime/runtime pulled into Next?

timneutkens commented 4 years ago

@alexparish regenerator-runtime is added when Next.js transforms async/await to regenerator.

balazsorban44 commented 2 years ago

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.