From e9d23d709cdc0d64ad22277a4eb782c6261dd0fa Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 1 Aug 2022 16:21:42 +0200 Subject: [PATCH] fix: react dev bundle is picked in prod mode (#39221) When we detect if `reactRoot` rendering should be enabled we `require` the require to check the version. But at that time the `NODE_ENV` isn't set yet. Then the react dev build stays in the `require.cache` that any future require of react will get the wrong build. In that case, React dev bundle is picked in production mode. Fun fact: if you're using hooks, that seem not to effect you, but context consumer works different then you couldn't get the proper context from provider. Fixes #38176 Fixes #38765 Fixes #38332 ## Bug - [x] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` --- packages/next/bin/next.ts | 4 +- .../ssr-react-context/app}/context.js | 0 .../ssr-react-context/app}/pages/_app.js | 0 .../ssr-react-context/app/pages/consumer.js | 17 +++++ .../ssr-react-context/app}/pages/index.js | 0 test/e2e/ssr-react-context/index.test.ts | 46 +++++++++++++ test/integration/ssr-ctx/test/index.test.js | 65 ------------------- 7 files changed, 66 insertions(+), 66 deletions(-) rename test/{integration/ssr-ctx => e2e/ssr-react-context/app}/context.js (100%) rename test/{integration/ssr-ctx => e2e/ssr-react-context/app}/pages/_app.js (100%) create mode 100644 test/e2e/ssr-react-context/app/pages/consumer.js rename test/{integration/ssr-ctx => e2e/ssr-react-context/app}/pages/index.js (100%) create mode 100644 test/e2e/ssr-react-context/index.test.ts delete mode 100644 test/integration/ssr-ctx/test/index.test.js diff --git a/packages/next/bin/next.ts b/packages/next/bin/next.ts index dd467cb224..65701856f1 100755 --- a/packages/next/bin/next.ts +++ b/packages/next/bin/next.ts @@ -2,7 +2,6 @@ import * as log from '../build/output/log' import arg from 'next/dist/compiled/arg/index.js' import { NON_STANDARD_NODE_ENV } from '../lib/constants' -import { shouldUseReactRoot } from '../server/utils' ;['react', 'react-dom'].forEach((dependency) => { try { // When 'npm link' is used it checks the clone location. Not the project. @@ -107,6 +106,9 @@ if (process.env.NODE_ENV) { ;(process.env as any).NODE_ENV = process.env.NODE_ENV || defaultEnv ;(process.env as any).NEXT_RUNTIME = 'nodejs' +// In node.js runtime, react has to be required after NODE_ENV is set, +// so that the correct dev/prod bundle could be loaded into require.cache. +const { shouldUseReactRoot } = require('../server/utils') if (shouldUseReactRoot) { ;(process.env as any).__NEXT_REACT_ROOT = 'true' } diff --git a/test/integration/ssr-ctx/context.js b/test/e2e/ssr-react-context/app/context.js similarity index 100% rename from test/integration/ssr-ctx/context.js rename to test/e2e/ssr-react-context/app/context.js diff --git a/test/integration/ssr-ctx/pages/_app.js b/test/e2e/ssr-react-context/app/pages/_app.js similarity index 100% rename from test/integration/ssr-ctx/pages/_app.js rename to test/e2e/ssr-react-context/app/pages/_app.js diff --git a/test/e2e/ssr-react-context/app/pages/consumer.js b/test/e2e/ssr-react-context/app/pages/consumer.js new file mode 100644 index 0000000000..ab8100bff2 --- /dev/null +++ b/test/e2e/ssr-react-context/app/pages/consumer.js @@ -0,0 +1,17 @@ +import React from 'react' + +const NumberContext = React.createContext(0) + +export default function page() { + return ( + + + {(value) =>

Value: {value}

} + + + ) +} + +export async function getServerSideProps() { + return { props: {} } +} diff --git a/test/integration/ssr-ctx/pages/index.js b/test/e2e/ssr-react-context/app/pages/index.js similarity index 100% rename from test/integration/ssr-ctx/pages/index.js rename to test/e2e/ssr-react-context/app/pages/index.js diff --git a/test/e2e/ssr-react-context/index.test.ts b/test/e2e/ssr-react-context/index.test.ts new file mode 100644 index 0000000000..503fe7f852 --- /dev/null +++ b/test/e2e/ssr-react-context/index.test.ts @@ -0,0 +1,46 @@ +import { join } from 'path' +import { renderViaHTTP, check } from 'next-test-utils' +import { NextInstance } from 'test/lib/next-modes/base' +import { createNext, FileRef } from 'e2e-utils' + +describe('React Context', () => { + let next: NextInstance + + beforeAll(async () => { + next = await createNext({ + files: { + pages: new FileRef(join(__dirname, 'app/pages')), + 'context.js': new FileRef(join(__dirname, 'app/context.js')), + }, + }) + }) + afterAll(() => next.destroy()) + + it('should render a page with context', async () => { + const html = await renderViaHTTP(next.url, '/') + expect(html).toMatch(/Value: .*?hello world/) + }) + + it('should render correctly with context consumer', async () => { + const html = await renderViaHTTP(next.url, '/consumer') + expect(html).toMatch(/Value: .*?12345/) + }) + + if ((globalThis as any).isNextDev) { + it('should render with context after change', async () => { + const aboutAppPagePath = 'pages/_app.js' + const originalContent = await next.readFile(aboutAppPagePath) + await next.patchFile( + aboutAppPagePath, + originalContent.replace('hello world', 'new value') + ) + + try { + await check(() => renderViaHTTP(next.url, '/'), /Value: .*?new value/) + } finally { + await next.patchFile(aboutAppPagePath, originalContent) + } + await check(() => renderViaHTTP(next.url, '/'), /Value: .*?hello world/) + }) + } +}) diff --git a/test/integration/ssr-ctx/test/index.test.js b/test/integration/ssr-ctx/test/index.test.js deleted file mode 100644 index c85bf57b07..0000000000 --- a/test/integration/ssr-ctx/test/index.test.js +++ /dev/null @@ -1,65 +0,0 @@ -/* eslint-env jest */ - -import { join } from 'path' -import { - File, - killApp, - findPort, - nextStart, - nextBuild, - renderViaHTTP, - check, - launchApp, -} from 'next-test-utils' - -const appDir = join(__dirname, '../') -const appPg = new File(join(appDir, 'pages/_app.js')) - -let appPort -let app - -const runTests = (isDev) => { - it('should render a page with context', async () => { - const html = await renderViaHTTP(appPort, '/') - expect(html).toMatch(/Value: .*?hello world/) - }) - - if (isDev) { - it('should render with context after change', async () => { - appPg.replace('hello world', 'new value') - - try { - await check(() => renderViaHTTP(appPort, '/'), /Value: .*?new value/) - } finally { - appPg.restore() - } - await check(() => renderViaHTTP(appPort, '/'), /Value: .*?hello world/) - }) - } -} - -describe('React Context', () => { - describe('dev mode', () => { - beforeAll(async () => { - appPort = await findPort() - app = await launchApp(appDir, appPort) - }) - afterAll(async () => { - await killApp(app) - appPg.restore() - }) - - runTests(true) - }) - - describe('production mode', () => { - beforeAll(async () => { - await nextBuild(appDir) - appPort = await findPort() - app = await nextStart(appDir, appPort) - }) - afterAll(() => killApp(app)) - - runTests() - }) -})