-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Build: Fix script_debug modules and scripts #72485
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.
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
Conversation
|
Size Change: -18.5 kB (-0.85%) Total Size: 2.15 MB
ℹ️ View Unchanged
|
20c57db to
60a652a
Compare
d76eee2 to
ce1fb70
Compare
| const getDefine = ( scriptDebug ) => ( { | ||
| ...baseDefine, | ||
| 'globalThis.SCRIPT_DEBUG': JSON.stringify( ! scriptDebug ), | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const getDefine = ( scriptDebug ) => ( { | |
| ...baseDefine, | |
| 'globalThis.SCRIPT_DEBUG': JSON.stringify( ! scriptDebug ), | |
| } ); | |
| const getDefine = ( minify ) => ( { | |
| ...baseDefine, | |
| 'globalThis.SCRIPT_DEBUG': JSON.stringify( ! minify ), | |
| } ); |
Nit: Let's make it more obvious that value is connected to build state. It also odd that scriptDebug: false means globalThis.SCRIPT_DEBUG: true 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what, claude wrote the code with "minify" but I renamed it because for the function itself, it's not really about minification haha :)
So what if if I invert the condition and values we pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works as well, less mental gymnastics :)
packages/interactivity/src/index.ts
Outdated
| export { useState, useRef } from 'preact/hooks'; | ||
|
|
||
| if ( globalThis.SCRIPT_DEBUG ) { | ||
| import( 'preact/debug' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preact/debug import needs to be static and be the first import of the application so I'm afraid we can't do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move it to the top, but it seems that this might work. Looking at the output JS it's bundled within the script so I'm wondering how I can test to ensure that it's working as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this, you can test by dumping something like the following in the interactivity code
class TestDebug extends Component {
constructor( props ) {
super( props );
this.setState( { test: true } ); // Will trigger warning
}
render() {
return null;
}
}
render( h( TestDebug ), document.createElement( 'div' ) );
you'll see a debug trace by preact.
ced985d to
e52c7f4
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
e52c7f4 to
4cfbf0b
Compare
|
Flaky tests detected in 4cfbf0b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18689812549
|
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that SCRIPT_DEBUG works as before ✅
I'll defer to @luisherranz regarding Interactivity API changes.
|
Import declarations are hoisted, so even though the new @DAreRodz, when you have a moment, can you check if this change seems to work for our debugging cases? I'm asking you because you are the one who has been using the Preact debugger in the DevTools most intensely. Thanks! |
|
Thanks for the ping, @luisherranz! I've been testing this PR, and the Preact Dev Tools seem to work the same as before. 🙂 Which is interesting; I believe I already tried the dynamic-import approach back when I was working on #60514, and faced some issues that led me to add an extra bundle for the debug version. My only concern is whether we've introduced any issues with the iAPI initialization. When we add a top-level PS: If you interact with the Preact Dev Tools, you could sometimes see errors in the console. These errors are also visible in |
|
Thanks David!
In principle, there shouldn't be any script module that depends on another module running before it without having an explicit dependency on it, including Or do you have any use case in mind where this could be a problem? |
Nope, I don't have any use case in mind right now. 😄 |
| } | ||
| break; | ||
| } | ||
| $script_module_id = '@wordpress/' . preg_replace( '~(?:/index)?\.(min\.)?js$~D', '', $file_name, 1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change meaningful? It seems to make a non-capture group capturing (min.) and it moves a literal ., ultimately resulting in analogous matching (I believe the capture group remains unused).
|
This broke an expectation in Core, changes are required there. See WordPress/wordpress-develop#10551. |
The PR changed the exports from the @wordpress/interactivity package. It removed the `debug` WordPress export module completely. This requires some changes to the special way the debug script module was being registered. See WordPress/gutenberg#72485.
Related #72032
What
Update the build system to generate both minified and non-minified versions of script modules, matching the pattern used for regular scripts. Also fixes the scripts injection of
globalThis.SCRIPT_DEBUGWhy?
Script modules were only generating .min.js files, while regular scripts generate both .js and .min.js. This inconsistency meant that:
This also allows us to remove the special case we had for interactivity API.
How?
Build system (bin/packages/build.mjs):
PHP registration (lib/client-assets.php):
Interactivity package cleanup:
Testing