Trigger page register when module is executed (#5115)

Solves inconsistent loading issues when one bundle is loaded faster than the other

Fixes zeit/next-plugins#244
Fixes #4997
Fixes #4620

build/webpack/plugins/pages-plugin.js

@@ -8,11 +8,24 @@ import {
 export default class PagesPlugin {
   apply (compiler: any) {
     compiler.hooks.compilation.tap('PagesPlugin', (compilation) => {
-      compilation.chunkTemplate.hooks.render.tap('PagesPluginRenderPageRegister', (modules, chunk) => {
-        if (!IS_BUNDLED_PAGE_REGEX.test(chunk.name)) {
-          return modules
+      // This hook is triggered right before a module gets wrapped into it's initializing function,
+      // For example when you look at the source of a bundle you'll see an object holding `'pages/_app.js': function(module, etc, etc)`
+      // This hook triggers right before that code is added and wraps the module into `__NEXT_REGISTER_PAGE` when the module is a page
+      // The reason we're doing this is that we don't want to execute the page code which has potential side effects before switching to a route
+      compilation.moduleTemplates.javascript.hooks.render.tap('PagesPluginRenderPageRegister', (moduleSourcePostModule, module, options) => {
+        const {chunk} = options
+
+        // check if the current module is the entry module, we only want to wrap the topmost module
+        if (chunk.entryModule !== module) {
+          return moduleSourcePostModule
         }
 
+        // Check if the chunk is a page
+        if (!IS_BUNDLED_PAGE_REGEX.test(chunk.name)) {
+          return moduleSourcePostModule
+        }
+
+        // Match the route the chunk belongs to
         let routeName = ROUTE_NAME_REGEX.exec(chunk.name)[1]
 
         // We need to convert \ into / when we are in windows
@@ -27,16 +40,12 @@ export default class PagesPlugin {
 
         routeName = `/${routeName.replace(/(^|\/)index$/, '')}`
 
-        const source = new ConcatSource()
-
-        source.add(`__NEXT_REGISTER_PAGE('${routeName}', function() {
-            var comp =
-        `)
-        source.add(modules)
-        source.add(`
-            return { page: comp.default }
-          })
-        `)
+        const source = new ConcatSource(
+          `__NEXT_REGISTER_PAGE('${routeName}', function() {\n`,
+          moduleSourcePostModule,
+          '\nreturn { page: module.exports.default }',
+          '});'
+        )
 
         return source
       })

client/webpack-hot-middleware-client.js

@@ -4,6 +4,12 @@ import Router from '../lib/router'
 
 const handlers = {
   reload (route) {
+    // If the App component changes we have to reload the current route, this is handled by hot-self-accept-loader
+    // So we just return
+    if (route === '/_app') {
+      return
+    }
+
     if (route === '/_error') {
       for (const r of Object.keys(Router.components)) {
         const { err } = Router.components[r]
@@ -16,12 +22,6 @@ const handlers = {
       return
     }
 
-    // If the App component changes we have to reload the current route
-    if (route === '/_app') {
-      Router.reload(Router.route)
-      return
-    }
-
     // Since _document is server only we need to reload the full page when it changes.
     if (route === '/_document') {
       window.location.reload()
@@ -32,9 +32,9 @@ const handlers = {
   },
 
   change (route) {
-    // If the App component changes we have to reload the current route
+    // If the App component changes we have to reload the current route, this is handled by hot-self-accept-loader
+    // So we just return
     if (route === '/_app') {
-      Router.reload(Router.route)
       return
     }
 

test/integration/production-config/next.config.js

@@ -0,0 +1,23 @@
+const withCSS = require('@zeit/next-css')
+const withSass = require('@zeit/next-sass')
+const path = require('path')
+module.exports = withCSS(withSass({
+  onDemandEntries: {
+    // Make sure entries are not getting disposed.
+    maxInactiveAge: 1000 * 60 * 60
+  },
+  webpack (config, {buildId}) {
+    // When next-css is `npm link`ed we have to solve loaders from the project root
+    const nextLocation = path.join(require.resolve('next/package.json'), '../')
+    const nextCssNodeModulesLocation = path.join(
+      require.resolve('@zeit/next-css'),
+      '../../../node_modules'
+    )
+
+    if (nextCssNodeModulesLocation.indexOf(nextLocation) === -1) {
+      config.resolveLoader.modules.push(nextCssNodeModulesLocation)
+    }
+
+    return config
+  }
+}))

test/integration/production-config/pages/_app.js

@@ -0,0 +1,15 @@
+import App, { Container } from 'next/app'
+import React from 'react'
+
+import '../styles.css'
+
+export default class MyApp extends App {
+  render () {
+    const { Component, pageProps } = this.props
+    return (
+      <Container>
+        <Component {...pageProps} />
+      </Container>
+    )
+  }
+}

test/integration/production-config/pages/index.js

@@ -0,0 +1,19 @@
+import React, {Component} from 'react'
+
+export default class extends Component {
+  constructor (props) {
+    super(props)
+
+    this.state = {
+      mounted: false
+    }
+  }
+
+  componentDidMount () {
+    this.setState({mounted: true})
+  }
+
+  render () {
+    return <p id='mounted'>ComponentDidMount {this.state.mounted ? 'executed on client' : 'not executed'}.</p>
+  }
+}

test/integration/production-config/styles.css

@@ -0,0 +1,4 @@
+p {
+  font-size: 40px;
+  color: red;
+}

test/integration/production-config/test/index.test.js

@@ -0,0 +1,59 @@
+/* global jasmine, describe, it, expect, beforeAll, afterAll */
+
+import { join } from 'path'
+import {
+  nextServer,
+  nextBuild,
+  startApp,
+  stopApp
+} from 'next-test-utils'
+import webdriver from 'next-webdriver'
+
+const appDir = join(__dirname, '../')
+let server
+let app
+jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 5
+
+const context = {}
+
+describe('Production Config Usage', () => {
+  beforeAll(async () => {
+    await nextBuild(appDir)
+    app = nextServer({
+      dir: join(__dirname, '../'),
+      dev: false,
+      quiet: true
+    })
+
+    server = await startApp(app)
+    context.appPort = server.address().port
+  })
+  afterAll(() => stopApp(server))
+
+  const openBrowser = (args) => webdriver(context.appPort, ...args)
+
+  describe('with next-css', () => {
+    it('should load styles', async () => {
+      let browser
+
+      async function testBrowser () {
+        browser = await openBrowser('/')
+        const element = await browser.elementByCss('#mounted')
+        const text = await element.text()
+        expect(text).toMatch(/ComponentDidMount executed on client\./)
+        expect(await element.getComputedCss('font-size')).toBe('40px')
+        expect(await element.getComputedCss('color')).toBe('rgba(255, 0, 0, 1)')
+      }
+      try {
+        // Try 3 times as the breaking happens intermittently
+        await testBrowser()
+        await testBrowser()
+        await testBrowser()
+      } finally {
+        if (browser) {
+          browser.close()
+        }
+      }
+    })
+  })
+})