Skip to content

Commit

Permalink
fix(next/babel): read env from caller, not process (#20679)
Browse files Browse the repository at this point in the history
This PR fixes a bug where `next/babel` would accidentally enable development transforms for a production build (`next build`).

This is tested by the two updated unit tests (which removed a workaround for this bug, and one now properly enables dev transforms).

---

Fixes #18929
Fixes #19001
x-ref #19046
x-ref #17032
  • Loading branch information
Timer authored Jan 2, 2021
1 parent 984ac96 commit 58ea3bb
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 24 deletions.
19 changes: 15 additions & 4 deletions packages/next/build/babel/preset.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { PluginItem } from 'next/dist/compiled/babel/core'
import { dirname } from 'path'
const env = process.env.NODE_ENV
const isProduction = env === 'production'
const isDevelopment = env === 'development'
const isTest = env === 'test'

const isLoadIntentTest = process.env.NODE_ENV === 'test'
const isLoadIntentDevelopment = process.env.NODE_ENV === 'development'

type StyledJsxPlugin = [string, any] | string
type StyledJsxBabelOptions =
Expand Down Expand Up @@ -65,6 +64,18 @@ module.exports = (
): BabelPreset => {
const supportsESM = api.caller(supportsStaticESM)
const isServer = api.caller((caller: any) => !!caller && caller.isServer)
const isCallerDevelopment = api.caller((caller: any) => caller?.isDev)

// Look at external intent if used without a caller (e.g. via Jest):
const isTest = isCallerDevelopment == null && isLoadIntentTest

// Look at external intent if used without a caller (e.g. Storybook):
const isDevelopment =
isCallerDevelopment === true ||
(isCallerDevelopment == null && isLoadIntentDevelopment)

// Default to production mode if not `test` nor `development`:
const isProduction = !(isTest || isDevelopment)

const useJsxRuntime =
options['preset-react']?.runtime === 'automatic' ||
Expand Down
9 changes: 5 additions & 4 deletions packages/next/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,6 @@ export default async function getBaseWebpackConfig(
: [require('pnp-webpack-plugin')],
}

const webpackMode = dev ? 'development' : 'production'

const terserOptions: any = {
parse: {
ecma: 8,
Expand Down Expand Up @@ -947,7 +945,10 @@ export default async function getBaseWebpackConfig(
[`process.env.${key}`]: JSON.stringify(config.env[key]),
}
}, {}),
'process.env.NODE_ENV': JSON.stringify(webpackMode),
// TODO: enforce `NODE_ENV` on `process.env`, and add a test:
'process.env.NODE_ENV': JSON.stringify(
dev ? 'development' : 'production'
),
'process.env.__NEXT_CROSS_ORIGIN': JSON.stringify(crossOrigin),
'process.browser': JSON.stringify(!isServer),
'process.env.__NEXT_TEST_MODE': JSON.stringify(
Expand Down Expand Up @@ -1013,7 +1014,7 @@ export default async function getBaseWebpackConfig(
: undefined),
// stub process.env with proxy to warn a missing value is
// being accessed in development mode
...(config.experimental.pageEnv && process.env.NODE_ENV !== 'production'
...(config.experimental.pageEnv && dev
? {
'process.env': `
new Proxy(${isServer ? 'process.env' : '{}'}, {
Expand Down
11 changes: 2 additions & 9 deletions test/unit/next-babel-loader.unit.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
/* eslint-env jest */

// avoid generating __source annotations in JSX during testing:
const NODE_ENV = process.env.NODE_ENV
process.env.NODE_ENV = 'production'
require('next/dist/build/babel/preset')
process.env.NODE_ENV = NODE_ENV

function interopRequireDefault(mod) {
return mod.default || mod
}
Expand Down Expand Up @@ -370,8 +363,8 @@ describe('next-babel-loader', () => {
{ resourcePath: pageFile, isServer: false, development: true }
)

expect(output).toMatchInlineSnapshot(
`"var __jsx=React.createElement;import React from\\"react\\";export var __N_SSG=true;export default function Home(_ref){var greeting=_ref.greeting;return __jsx(\\"h1\\",null,greeting);}_c=Home;var _c;$RefreshReg$(_c,\\"Home\\");"`
expect(output).toMatch(
/var _jsxFileName="[^"]+";var __jsx=React\.createElement;import React from"react";export var __N_SSG=true;export default function Home\(_ref\)\{var greeting=_ref\.greeting;return __jsx\("h1",\{__self:this,__source:\{fileName:_jsxFileName,lineNumber:8,columnNumber:20\}\},greeting\);\}_c=Home;var _c;\$RefreshReg\$\(_c,"Home"\);/
)
})

Expand Down
9 changes: 2 additions & 7 deletions test/unit/next-babel.unit.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,18 @@ import { transform } from '@babel/core'

const trim = (s) => s.join('\n').trim().replace(/^\s+/gm, '')

// avoid generating __source annotations in JSX during testing:
const NODE_ENV = process.env.NODE_ENV
process.env.NODE_ENV = 'production'
const preset = require('next/dist/build/babel/preset')
process.env.NODE_ENV = NODE_ENV

const babel = (code, esm = false, presetOptions = {}, filename = 'noop.js') =>
transform(code, {
filename,
presets: [[preset, presetOptions]],
presets: [[require('next/dist/build/babel/preset'), presetOptions]],
babelrc: false,
configFile: false,
sourceType: 'module',
compact: true,
caller: {
name: 'tests',
supportsStaticESM: esm,
isDev: false,
},
}).code

Expand Down

0 comments on commit 58ea3bb

Please sign in to comment.