-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Script Loader: Add sourceURL comment (attempt 2). #9672
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
This reverts commit 2fe30e7.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
LGTM
In if ( $concat ) {
if ( ! empty( $wp_scripts->print_code ) ) {
$inline_tag_id = wp_unique_prefixed_id( 'wp-concatenated-inline-javascript-' );
echo "\n<script{$type_attr} id=\"{$inline_tag_id}\">\n";
echo "/* <![CDATA[ */\n"; // Not needed in HTML 5.
echo $wp_scripts->print_code;
echo "/* ]]> */\n";
echo "//# sourceURL={$inline_tag_id}\n";
echo "</script>\n";
} |
I think we should rather eliminate script/style concatenation per Core-57548. |
That would be consistent with the rest of this change, it seems good 👍
Whatever is done here for |
echo "\n<script{$type_attr}>\n"; | ||
echo "/* <![CDATA[ */\n"; // Not needed in HTML 5. | ||
echo $wp_scripts->print_code; | ||
echo sprintf( "\n//# sourceURL=%s\n", rawurlencode( 'js-inline-concat-' . $concat ) ); |
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.
There's example output in tests, but this outputs a sourceURL like this:
//# sourceURL=js-inline-concat-one%2Ctwo
$concat
is a list of the concatenated script names, one,two
in this case. It should work well as a descriptive identifier. For example, on my test site on the wp-admin/customizer.php
page I get an inline script that's identified in the Chrome devtools source panel as js-inline-concat-heartbeat,customize-widgets,react,react-dom,react-jsx-runtime,wp-polyfill,wp-url
.
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 like this, even if it is short-lived. 👍🏻
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 asked Gemini CLI to review this:
gh pr checkout https://github.com/WordPress/wordpress-develop/pull/9672
git diff origin/trunk... | gemini -m "gemini-2.5-pro" -p "Review these changes to WordPress core according to WordPress coding standards. Anything amiss?"
It noticed something:
Yes, there are a couple of things amiss in the test updates. While the core logic looks sound, some of the PHPUnit tests have not been updated completely.
In
tests/phpunit/tests/dependencies/scripts.php
, two tests have inconsistencies in their expected output:
- In
test_wp_add_inline_script_before_for_two_scripts
, the expected output for the inline script with handleone-js-before
is missing the//# sourceURL
comment, but it is present fortwo-js-before
.- Similarly, in
test_wp_add_inline_script_after_for_two_scripts
, the expected output fortwo-js-after
is missing the//# sourceURL
comment, whilethree-js-after
has it.These seem to be oversights while updating the tests. The expected output for all inline scripts should be updated to include the new
sourceURL
comment to accurately reflect the changes.Otherwise, the changes to the core files (
class-wp-scripts.php
,class-wp-styles.php
,script-loader.php
) and the new tests you've added look correct and adhere to WordPress coding standards.
In evaluating its response, it does seem like perhaps some sourceURL
comments are missing.
wp_add_inline_script( 'one', 'console.log("before one");', 'before' ); | ||
wp_add_inline_script( 'two', 'console.log("before two");', 'before' ); | ||
|
||
$expected = "<script type='text/javascript' id='one-js-before'>\n/* <![CDATA[ */\nconsole.log(\"before one\");\n/* ]]> */\n</script>\n"; |
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.
Why doesn't this include //# sourceURL=one-js-before
?
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.
Yeah… this is interesting to dig into.
I think what's happening is that before/after scripts are never actually concatenated. I struggled trying to write tests for their concatenation and never succeeded, then I looked at the logic and found this.
Before and after script tags are created here (with do_concat
untouched):
wordpress-develop/src/wp-includes/class-wp-scripts.php
Lines 327 to 334 in 6fb4a2f
$before_script = $this->get_inline_script_tag( $handle, 'before' ); | |
$after_script = $this->get_inline_script_tag( $handle, 'after' ); | |
if ( $before_script || $after_script ) { | |
$inline_script_tag = $ie_conditional_prefix . $before_script . $after_script . $ie_conditional_suffix; | |
} else { | |
$inline_script_tag = ''; | |
} |
A few lines later it turns off concat and continues if there are before/after scripts:
wordpress-develop/src/wp-includes/class-wp-scripts.php
Lines 347 to 362 in 6fb4a2f
if ( $this->do_concat ) { | |
/** | |
* Filters the script loader source. | |
* | |
* @since 2.2.0 | |
* | |
* @param string $src Script loader source path. | |
* @param string $handle Script handle. | |
*/ | |
$filtered_src = apply_filters( 'script_loader_src', $src, $handle ); | |
if ( | |
$this->in_default_dir( $filtered_src ) | |
&& ( $before_script || $after_script || $translations_stop_concat || $this->is_delayed_strategy( $strategy ) ) | |
) { | |
$this->do_concat = false; |
But extra scripts (wp_localize_script()
) respects concatenation logic:
wordpress-develop/src/wp-includes/class-wp-scripts.php
Lines 367 to 368 in 6fb4a2f
} elseif ( $this->in_default_dir( $filtered_src ) && ! $conditional ) { | |
$this->print_code .= $this->print_extra_script( $handle, false ); |
As seemed perfectly reasonable, I gated the sourceURL
printing in the before/after scripts do_concat
:
But this test is very strange! one-js-before
gets no sourceURL but two-js-before
does!
Reviewing the snippets above, it seems like WP_Scripts
sets do_concat = false
and never touches it again, I think changing that value is always done in script-loader.php
with the exception of this case and in ::reset()
:
wordpress-develop/src/wp-includes/class-wp-scripts.php
Lines 1048 to 1056 in 6fb4a2f
public function reset() { | |
$this->do_concat = false; | |
$this->print_code = ''; | |
$this->concat = ''; | |
$this->concat_version = ''; | |
$this->print_html = ''; | |
$this->ext_version = ''; | |
$this->ext_handles = ''; | |
} |
So when the first inline script is hit, it turns off concat and gives up, causing the rest of the before/after scripts to be printed with sourceURL.
So I think sourceURL
can always be printed inside of before/after scripts. I'll try that out.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Looks good! Just a couple minor suggestions that don't block commit.
Gemini also approves:
Based on my review of the provided diff, the changes appear to be well-implemented and adhere to WordPress coding standards. This is a solid enhancement for developer experience, especially for debugging.
Here’s a summary of my analysis:
What's Good
- Clear Improvement: Adding
sourceURL
to inline scripts and styles is a definite win for debugging, as it gives unnamed code blocks a clear identifier in browser developer tools.- Correct Implementation: The patch correctly uses
//# sourceURL=
for JavaScript and/*# sourceURL=... */
for CSS.- Concatenation Handling: The logic correctly avoids adding
sourceURL
to individual scripts/styles that are intended for concatenation. Instead, a singlesourceURL
is added to the concatenated block, which is the right approach.- Secure Encoding:
rawurlencode()
is used to encode the handle name, preventing issues with special characters in the URL.- Thorough Testing: The patch includes comprehensive updates to existing tests and adds new, specific tests for the new functionality. The new tests for
sourceURL
encoding (test_source_url_encoding
) and concatenation (test_source_url_with_concat
) are excellent additions that cover important edge cases.- Code Quality: The changes are clean, consistent, and follow WordPress conventions.
Anything Amiss?
No. The implementation is robust and the attention to detail is evident, especially in how concatenation is handled and in the new unit tests. The changes to the test suite are thorough and demonstrate the correctness of the implementation across many different script-loading scenarios.
This is a high-quality patch.
if ( empty( $data ) || ! is_array( $data ) ) { | ||
return ''; | ||
} | ||
|
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.
To contrast this with extra
above, it might be worth adding add a comment to note that inline before/after scripts do not get concatenated together, so it is fine to source URL 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.
👍 1bbecc8
$this->old_wp_styles = isset( $GLOBALS['wp_styles'] ) ? $GLOBALS['wp_styles'] : null; | ||
$this->old_wp_scripts = isset( $GLOBALS['wp_scripts'] ) ? $GLOBALS['wp_scripts'] : null; | ||
$this->old_wp_styles = isset( $GLOBALS['wp_styles'] ) ? $GLOBALS['wp_styles'] : null; | ||
$this->old_concatenate_scripts = isset( $GLOBALS['concatenate_scripts'] ) ? $GLOBALS['concatenate_scripts'] : null; |
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.
Good call.
public function test_source_url_encoding() { | ||
$this->add_html5_script_theme_support(); | ||
|
||
$handle = '# test/</script> #'; |
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 would!
wp_enqueue_script( 'one', $this->default_scripts_dir . '1.js' ); | ||
wp_enqueue_script( 'two', $this->default_scripts_dir . '2.js' ); | ||
wp_localize_script( 'one', 'one', array( 'key' => 'val' ) ); | ||
wp_localize_script( 'two', 'two', array( 'key' => 'val' ) ); |
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.
Would it make sense to throw in a couple inline before/after scripts here as well to test?
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'm not sure. When a script has associated inline scripts, it becomes ineligible for concat and is printed on its own.
With a change like this to tests:
diff --git c/tests/phpunit/tests/dependencies/scripts.php i/tests/phpunit/tests/dependencies/scripts.php
index 4f78caa765..0a485d9491 100644
--- c/tests/phpunit/tests/dependencies/scripts.php
+++ i/tests/phpunit/tests/dependencies/scripts.php
@@ -3804,9 +3804,13 @@ HTML;
wp_enqueue_script( 'two', $this->default_scripts_dir . '2.js' );
wp_localize_script( 'one', 'one', array( 'key' => 'val' ) );
wp_localize_script( 'two', 'two', array( 'key' => 'val' ) );
+ wp_add_inline_script( 'two', '/* 2.0 before */', 'before' );
+ wp_add_inline_script( 'two', '/* 2.1 before */', 'before' );
+ wp_add_inline_script( 'two', '/* 2.0 after */', 'after' );
+ wp_add_inline_script( 'two', '/* 2.1 after */', 'after' );
- wp_print_scripts();
- $print_scripts = get_echo( '_print_scripts' );
+ $print_scripts = get_echo( 'wp_print_scripts' );
+ $print_scripts .= get_echo( '_print_scripts' );
$expected = <<<HTML
<script>
It fails like this, where one
is "concatenated" (by itself 🤷) and two
is on printed as a non-concat script:
├ HTML markup was not equivalent.
├ Failed asserting that two strings are identical.
┊ ---·Expected
┊ +++·Actual
┊ @@ @@
┊ '<script>
┊ "
┊ /* <![CDATA[ */
┊ -var·one·=·{"key":"val"};var·two·=·{"key":"val"};
┊ -//#·sourceURL=js-inline-concat-one%2Ctwo
┊ +var·one·=·{"key":"val"};
┊ +//#·sourceURL=js-inline-concat-one
┊ /* ]]> */
┊ "
┊ <script>
┊ -··src="/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=one,two&ver=6.9-alpha-60093-src"
┊ +··src="/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=one&ver=6.9-alpha-60093-src"
┊ +<script>
┊ +··id="two-js-extra"
┊ +··"
┊ +var·two·=·{"key":"val"};
┊ +//#·sourceURL=two-js-extra
┊ +"
┊ +<script>
┊ +··id="two-js-before"
┊ +··"
┊ +/*·2.0·before·*/
┊ +/*·2.1·before·*/
┊ +//#·sourceURL=two-js-before
┊ +"
┊ +<script>
┊ +··id="two-js"
┊ +··src="/directory/2.js?ver=6.9-alpha-60093-src"
┊ +<script>
┊ +··id="two-js-after"
┊ +··"
┊ +/*·2.0·after·*/
┊ +/*·2.1·after·*/
┊ +//#·sourceURL=two-js-after
┊ +"
┊ '
Another script could be added here to verify its printed on its own, but that behavior is likely under test elsewhere in this file.
I'll land this with the existing tests, but I'm happy to follow-up with suggestions.
Improve the source locations referenced by developer tooling in supporting browsers. Inline source locations are named like inline:handle-js-after and appear in the developer tools "sources" panel. This is the second attempt to add sourceURL comments. The first attempt in [60685] was reverted due to an issue with script concatenation that has been addressed. Developed in #9672. Follow-up to [60685], [60690]. Props jonsurrell, westonruter, wildworks, peterwilsoncc, johnbillion, tobiasbg. Fixes #63887. git-svn-id: https://develop.svn.wordpress.org/trunk@60719 602fd350-edb4-49c9-b593-d223f7449a82
Improve the source locations referenced by developer tooling in supporting browsers. Inline source locations are named like inline:handle-js-after and appear in the developer tools "sources" panel. This is the second attempt to add sourceURL comments. The first attempt in [60685] was reverted due to an issue with script concatenation that has been addressed. Developed in WordPress/wordpress-develop#9672. Follow-up to [60685], [60690]. Props jonsurrell, westonruter, wildworks, peterwilsoncc, johnbillion, tobiasbg. Fixes #63887. Built from https://develop.svn.wordpress.org/trunk@60719 git-svn-id: http://core.svn.wordpress.org/trunk@60055 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Improve the source locations referenced by developer tooling in supporting browsers. Inline source locations are named like inline:handle-js-after and appear in the developer tools "sources" panel. This is the second attempt to add sourceURL comments. The first attempt in [60685] was reverted due to an issue with script concatenation that has been addressed. Developed in WordPress/wordpress-develop#9672. Follow-up to [60685], [60690]. Props jonsurrell, westonruter, wildworks, peterwilsoncc, johnbillion, tobiasbg. Fixes #63887. Built from https://develop.svn.wordpress.org/trunk@60719 git-svn-id: https://core.svn.wordpress.org/trunk@60055 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Improve the source locations referenced by developer tooling in supporting browsers. Inline source locations are named like inline:handle-js-after and appear in the developer tools "sources" panel. This is the second attempt to add sourceURL comments. The first attempt in [60685] was reverted due to an issue with script concatenation that has been addressed. Developed in WordPress#9672. Follow-up to [60685], [60690]. Props jonsurrell, westonruter, wildworks, peterwilsoncc, johnbillion, tobiasbg. Fixes #63887. git-svn-id: https://develop.svn.wordpress.org/trunk@60719 602fd350-edb4-49c9-b593-d223f7449a82
Apply the patch to add
sourceURL
comments to inline scripts and styles (revert #9671):New in this PR
Disable
sourceURL
comments when concatenation is enabled. This is problematic for two reasons://
line comments, breaking the output. This happens with "extra" scripts, i.e.wp_localize_script()
.sourceURL
comments are simply disabled.Testing
In the initial version, the
sourceURL
comments would break thewp_localize_script
necessary for the customizer page. In this PR it should work correctly. Test as follows:See https://core.trac.wordpress.org/ticket/63887#comment:16.
The PR includes tests to verify
sourceURL
comments are not added when concat is enabled.Trac ticket: https://core.trac.wordpress.org/ticket/63887
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.