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.

Sign up for GitHub

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

Warning: Introduce SCRIPT_DEBUG to make the package compatible with webpack 5 #50122

Merged
merged 3 commits into from
Aug 18, 2023
Merged

Warning: Introduce SCRIPT_DEBUG to make the package compatible with webpack 5 #50122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abbfb88
Warning: Introduce `SCRIPT_DEBUG` to make the package compatible with…
gziolo Apr 27, 2023
f552753
Improve the check for SCRIPT_DEBUG so it account for being undefined
gziolo Apr 27, 2023
1fe96b6
Update packages/scripts/config/webpack.config.js
gziolo Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/eslint-plugin/configs/recommended-with-formatting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const config = {
globals: {
window: true,
document: true,
SCRIPT_DEBUG: 'readonly',
wp: 'readonly',
},
settings: {
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-preset-default/scripts/setup-globals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Run all tests with development tools enabled.
global.SCRIPT_DEBUG = true;

// These are necessary to load TinyMCE successfully.
global.URL = window.URL;
global.window.tinyMCEPreInit = {
Expand Down
5 changes: 5 additions & 0 deletions packages/scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
const { BundleAnalyzerPlugin } = require( 'webpack-bundle-analyzer' );
const { CleanWebpackPlugin } = require( 'clean-webpack-plugin' );
const CopyWebpackPlugin = require( 'copy-webpack-plugin' );
const { DefinePlugin } = require( 'webpack' );
const browserslist = require( 'browserslist' );
const MiniCSSExtractPlugin = require( 'mini-css-extract-plugin' );
const { basename, dirname, resolve } = require( 'path' );
Expand Down Expand Up @@ -243,6 +244,10 @@ const config = {
],
},
plugins: [
new DefinePlugin( {
// Inject the `SCRIPT_DEBUG` global, used for development features flagging.
SCRIPT_DEBUG: ! isProduction,
} ),
// During rebuilds, all webpack assets that are not used anymore will be
// removed automatically. There is an exception added in watch mode for
// fonts and images. It is a known limitations:
Expand Down
2 changes: 1 addition & 1 deletion packages/warning/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ To prevent that, you should:

1. Put `@wordpress/warning/babel-plugin` into your [babel config](https://babeljs.io/docs/en/plugins#plugin-options) or use [`@wordpress/babel-preset-default`](https://www.npmjs.com/package/@wordpress/babel-preset-default), which already includes the babel plugin.

This will make sure your `warning` calls are wrapped within a condition that checks if `process.env.NODE_ENV !== 'production'`.
This will make sure your `warning` calls are wrapped within a condition that checks if `SCRIPT_DEBUG === true`.

2. Use [UglifyJS](https://github.com/mishoo/UglifyJS2), [Terser](https://github.com/terser/terser) or any other JavaScript parser that performs [dead code elimination](https://en.wikipedia.org/wiki/Dead_code_elimination). This is usually used in conjunction with JavaScript bundlers, such as [webpack](https://github.com/webpack/webpack).

Expand Down
32 changes: 9 additions & 23 deletions packages/warning/babel-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const pkg = require( './package.json' );

/**
* Babel plugin which transforms `warning` function calls to wrap within a
* condition that checks if `process.env.NODE_ENV !== 'production'`.
* condition that checks if `SCRIPT_DEBUG === true`.
*
* @param {import('@babel/core')} babel Current Babel object.
*
Expand All @@ -16,34 +16,20 @@ function babelPlugin( { types: t } ) {

const typeofProcessExpression = t.binaryExpression(
'!==',
t.unaryExpression( 'typeof', t.identifier( 'process' ), false ),
t.unaryExpression( 'typeof', t.identifier( 'SCRIPT_DEBUG' ), false ),
t.stringLiteral( 'undefined' )
);

const processEnvExpression = t.memberExpression(
t.identifier( 'process' ),
t.identifier( 'env' ),
false
);

const nodeEnvCheckExpression = t.binaryExpression(
'!==',
t.memberExpression(
processEnvExpression,
t.identifier( 'NODE_ENV' ),
false
),
t.stringLiteral( 'production' )
const scriptDebugCheckExpression = t.binaryExpression(
'===',
t.identifier( 'SCRIPT_DEBUG' ),
t.booleanLiteral( true )
);

const logicalExpression = t.logicalExpression(
'&&',
t.logicalExpression(
'&&',
typeofProcessExpression,
processEnvExpression
),
nodeEnvCheckExpression
typeofProcessExpression,
scriptDebugCheckExpression
);

return {
Expand Down Expand Up @@ -80,7 +66,7 @@ function babelPlugin( { types: t } ) {
// Turns this code:
// warning(argument);
// into this:
// typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning(argument) : void 0;
// typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true ? warning(argument) : void 0;
node[ seen ] = true;
path.replaceWith(
t.ifStatement(
Expand Down
6 changes: 1 addition & 5 deletions packages/warning/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
import { logged } from './utils';

function isDev() {
return (
typeof process !== 'undefined' &&
process.env &&
process.env.NODE_ENV !== 'production'
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the process polyfill is no longer present, it shouldn't matter for process.env.NODE_ENV, because it's a special constant defined and replaced by the DefinePlugin. This plugin will expand

process.env.NODE_ENV !== 'production'

or

process?.env?.NODE_ENV !== 'production'

into

'development' !== 'production'

eliminating the process reference from the output.

But the DefinePlugin doesn't undestand what process or process.env is and will leave it in the output.

Therefore, the easiest fix for the bug is to use:

return ( process?.env?.NODE_ENV ?? 'production' ) !== 'production';

Here DefinePlugin can do the expansion and keeps the logic where:

  • if process or process.env doesn't exist, isDev is always false.
  • otherwise isDev is true iff NODE_ENV is not production.

Adding support for SCRIPT_DEBUG is nice, but I think it's a mistake to assume that process.env.NODE_ENV is bad and no longer supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, the easiest fix for the bug is to use:

return ( process?.env?.NODE_ENV ?? ‘production’ ) !== ‘production’;

Yes, but it only fixes the bug for the @wordpress/warning usage with the integrated Babel plugin. It doesn’t help with the case where someone wants to use process.env.NODE_ENV in other packages to guard the usage of code. They would have to use something like ( process?.env?.NODE_ENV ?? ‘production’ ) !== ‘production’ to avoid errors when the DefinePlugin is not replacing the entry. Therefore I think that SCRIPT_DEBUG is way simpler to use. Folks outside of WordPress core and the Gutenberg plugin can use the longer version with DefinePlugin, if they prefer it over SCRIPT_DEBUG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid errors when the DefinePlugin is not replacing the entry.

When would such an error happen? It won't happen in Node.js, and it won't happen in webpack (if process.env.NODE_ENV is written in such a way that DefinePlugin can fully replace it). It can happen in a browser, when the @wordpress/warning package is imported with the native ES module loader. Is that the case we try to guard against here?

It's increasingly common that packages ship separate ES modules for Node and for browsers. Specified in the exports field. If we decided to do that, too, presence of process.env.NODE_ENV would be on of the differences.

When testing this PR, I see one odd thing: the built script for the warning package, in build/warning/index.js, the SCRIPT_DEBUG variable is replaced away, and in production version the warning function is empty. SCRIPT_DEBUG global will never enable it again.

So, do we really want to specify SCRIPT_DEBUG with DefinePlugin? Wouldn't it be better to keep the references to the global in the built script?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the SCRIPT_DEBUG variable is replaced away

that is the expected behavior with this plugin, yes. It‘s a global that‘s compiled away during build

Copy link
Member Author

@gziolo gziolo May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid errors when the DefinePlugin is not replacing the entry.

When would such an error happen? It won't happen in Node.js, and it won't happen in webpack (if process.env.NODE_ENV is written in such a way that DefinePlugin can fully replace it).

It's the bug that was filed initially in #44950. The current webpack configuration doesn't do anything with process.env.NODE_ENV.

When testing this PR, I see one odd thing: the built script for the warning package, in build/warning/index.js, the SCRIPT_DEBUG variable is replaced away, and in production version the warning function is empty. SCRIPT_DEBUG global will never enable it again.

Yes, that is expected as it aligns with how WordPress core loads files. See a more detailed explanation in #50122 (comment).

return typeof SCRIPT_DEBUG !== 'undefined' && SCRIPT_DEBUG === true;
}

/**
Expand Down
15 changes: 8 additions & 7 deletions packages/warning/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,28 @@
import warning from '..';
import { logged } from '../utils';

const initialNodeEnv = process.env.NODE_ENV;

describe( 'warning', () => {
const initialScriptDebug = global.SCRIPT_DEBUG;

afterEach( () => {
process.env.NODE_ENV = initialNodeEnv;
global.SCRIPT_DEBUG = initialScriptDebug;
logged.clear();
} );

it( 'logs to console.warn when NODE_ENV is not "production"', () => {
process.env.NODE_ENV = 'development';
it( 'logs to console.warn when SCRIPT_DEBUG is set to `true`', () => {
global.SCRIPT_DEBUG = true;
warning( 'warning' );
expect( console ).toHaveWarnedWith( 'warning' );
} );

it( 'does not log to console.warn if NODE_ENV is "production"', () => {
process.env.NODE_ENV = 'production';
it( 'does not log to console.warn if SCRIPT_DEBUG not set to `true`', () => {
global.SCRIPT_DEBUG = false;
warning( 'warning' );
expect( console ).not.toHaveWarned();
} );

it( 'should show a message once', () => {
global.SCRIPT_DEBUG = true;
warning( 'warning' );
warning( 'warning' );

Expand Down
22 changes: 11 additions & 11 deletions packages/warning/test/babel-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe( 'babel-plugin', () => {
);
const expected = join(
'import warning from "@wordpress/warning";',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning("a") : void 0;'
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warning("a") : void 0;'
);

expect( transformCode( input ) ).toEqual( expected );
Expand All @@ -45,7 +45,7 @@ describe( 'babel-plugin', () => {
const input = 'warning("a");';
const options = { callee: 'warning' };
const expected =
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning("a") : void 0;';
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warning("a") : void 0;';

expect( transformCode( input, options ) ).toEqual( expected );
} );
Expand All @@ -59,9 +59,9 @@ describe( 'babel-plugin', () => {
);
const expected = join(
'import warning from "@wordpress/warning";',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning("a") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning("b") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warning("c") : void 0;'
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warning("a") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warning("b") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warning("c") : void 0;'
);

expect( transformCode( input ) ).toEqual( expected );
Expand All @@ -76,9 +76,9 @@ describe( 'babel-plugin', () => {
);
const expected = join(
'import warn from "@wordpress/warning";',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("a") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("b") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("c") : void 0;'
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("a") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("b") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("c") : void 0;'
);

expect( transformCode( input ) ).toEqual( expected );
Expand All @@ -93,9 +93,9 @@ describe( 'babel-plugin', () => {
);
const expected = join(
'import warn from "@wordpress/warning";',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("a") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("b") : void 0;',
'typeof process !== "undefined" && process.env && process.env.NODE_ENV !== "production" ? warn("c") : void 0;'
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("a") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("b") : void 0;',
'typeof SCRIPT_DEBUG !== "undefined" && SCRIPT_DEBUG === true ? warn("c") : void 0;'
);

expect( transformCode( input ) ).toEqual( expected );
Expand Down
2 changes: 2 additions & 0 deletions tools/webpack/shared.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ const plugins = [
// Inject the `IS_WORDPRESS_CORE` global, used for feature flagging.
'process.env.IS_WORDPRESS_CORE':
process.env.npm_package_config_IS_WORDPRESS_CORE,
// Inject the `SCRIPT_DEBUG` global, used for dev versions of JavaScript.
SCRIPT_DEBUG: mode === 'development',
} ),
mode === 'production' && new ReadableJsAssetsWebpackPlugin(),
];
Expand Down
2 changes: 2 additions & 0 deletions typings/gutenberg-env/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ interface Process {
env: Environment;
}
declare var process: Process;

declare var SCRIPT_DEBUG: boolean;