Warning: Introduce SCRIPT_DEBUG to make the package compatible with webpack 5
#50122
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gziolo
added
[Package] Warning
gziolo
force-pushed
the
fix/warning-not-compatible-weback-5
branch
from
c696537
to
7065471
Compare
|
Size Change: -264 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
gziolo
commented
Apr 27, 2023
gziolo
requested review from
ntwb,
nerrad,
ajitbohra and
ryanwelcher
as code owners
|
It would also be great to figure out what would be the most suitable places to include a note about the possibility of using |
jsnajdr
reviewed
May 9, 2023
| typeof process !== 'undefined' && | ||
| process.env && | ||
| process.env.NODE_ENV !== 'production' | ||
| ); |
There was a problem hiding this comment.
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
processorprocess.envdoesn't exist,isDevis alwaysfalse. - otherwise
isDevistrueiffNODE_ENVis notproduction.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
to avoid errors when the
DefinePluginis 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
|
The implementation looks sound. I'm just not sure about re-using the same |
|
Should we consider setting the JavaScript |
While So this isn't actually a global that's exposed on the Hope that makes sense |
Yes, it's exactly how it works. It completely removes the code behind the flag when building for production. It aligns with how WordPress Core usually handles scripts using
In effect, the webpack config follows this pattern and produces two files for every script
That would be only useful in case someone doesn't use webpack to replace |
luisherranz
approved these changes
Jun 15, 2023
There was a problem hiding this comment.
This looks good to me, and we'll gladly use it once it's merged to enable the debug version of Preact in the Interactivity API.
7 tasks
gziolo
force-pushed
the
fix/warning-not-compatible-weback-5
branch
from
70ae293
to
38014af
Compare
gziolo
added
[Type] Bug
gziolo
force-pushed
the
fix/warning-not-compatible-weback-5
branch
from
38014af
to
aa0cb31
Compare
gziolo
force-pushed
the
fix/warning-not-compatible-weback-5
branch
from
aa0cb31
to
1fe96b6
Compare
|
I will follow up with documentation covering |
|
I opened a ticket in WordPress Trac to bring over the same changes to the webpack config: |
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Sep 26, 2023
Backports the same changes to the webpack config in the Gutenberg plugin with WordPress/gutenberg#50122. The `warning` function from `@wordpress/warning` no longer worked correctly with webpack 5. In practice, it no longer called `console.warn`. To fix it, the usage of `process.env.NODE_ENV` check got replaced with another optional global: `SCRIPT_DEBUG`. All the tools used in the Gutenberg, get updated to work with this new constant, including `@wordpress/scripts`. This way, developers are able to guard code that should be run only in development mode. In WordPress core, the same constant needs to be added mostly to ensure that the code behind the check gets completely removed in production mode. Fixes #59407. Built from https://develop.svn.wordpress.org/trunk@56699 git-svn-id: http://core.svn.wordpress.org/trunk@56211 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to platformsh/wordpress-performance
that referenced
this pull request
Sep 26, 2023
Backports the same changes to the webpack config in the Gutenberg plugin with WordPress/gutenberg#50122. The `warning` function from `@wordpress/warning` no longer worked correctly with webpack 5. In practice, it no longer called `console.warn`. To fix it, the usage of `process.env.NODE_ENV` check got replaced with another optional global: `SCRIPT_DEBUG`. All the tools used in the Gutenberg, get updated to work with this new constant, including `@wordpress/scripts`. This way, developers are able to guard code that should be run only in development mode. In WordPress core, the same constant needs to be added mostly to ensure that the code behind the check gets completely removed in production mode. Fixes #59407. Built from https://develop.svn.wordpress.org/trunk@56699 git-svn-id: https://core.svn.wordpress.org/trunk@56211 1a063a9b-81f0-0310-95a4-ce76da25c4cd
mikachan
pushed a commit
to mikachan/wordpress-develop
that referenced
this pull request
Sep 26, 2023
Backports the same changes to the webpack config in the Gutenberg plugin with WordPress/gutenberg#50122. The `warning` function from `@wordpress/warning` no longer worked correctly with webpack 5. In practice, it no longer called `console.warn`. To fix it, the usage of `process.env.NODE_ENV` check got replaced with another optional global: `SCRIPT_DEBUG`. All the tools used in the Gutenberg, get updated to work with this new constant, including `@wordpress/scripts`. This way, developers are able to guard code that should be run only in development mode. In WordPress core, the same constant needs to be added mostly to ensure that the code behind the check gets completely removed in production mode. Fixes #59407. git-svn-id: https://develop.svn.wordpress.org/trunk@56699 602fd350-edb4-49c9-b593-d223f7449a82
anton-vlasenko
pushed a commit
to anton-vlasenko/wordpress-develop
that referenced
this pull request
Sep 29, 2023
Backports the same changes to the webpack config in the Gutenberg plugin with WordPress/gutenberg#50122. The `warning` function from `@wordpress/warning` no longer worked correctly with webpack 5. In practice, it no longer called `console.warn`. To fix it, the usage of `process.env.NODE_ENV` check got replaced with another optional global: `SCRIPT_DEBUG`. All the tools used in the Gutenberg, get updated to work with this new constant, including `@wordpress/scripts`. This way, developers are able to guard code that should be run only in development mode. In WordPress core, the same constant needs to be added mostly to ensure that the code behind the check gets completely removed in production mode. Fixes #59407. git-svn-id: https://develop.svn.wordpress.org/trunk@56699 602fd350-edb4-49c9-b593-d223f7449a82
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #44950.
The
warningfrom@wordpress/warningno longer works correctly with webpack 5. In practice, it no longer callsconsole.warn.Why?
How?
We replace the usage of
process.env.NODE_ENVwith another optional constant:SCRIPT_DEBUG. All the tools used in the Gutenberg, get updated to work with this new constant, including@wordpress/scripts. This way, developers will be able to guard code that should be run only in development mode.The selection of
SCRIPT_DEBUGwas intentional to mirror the same constant used in WordPress Core that is described as:Here, we are extending its application to be always present and enabled in the
devversion of JavaScript.Testing Instructions
npm run dev.npm run build.