Build: Wrap script bundles in an IIFE to contain 'use strict' - #79792
Conversation
esbuild's `format: 'iife'` emits a `'use strict'` directive at the top of each package bundle. Because WordPress concatenates registered scripts by joining raw file contents (wp-admin/load-scripts.php), a file-level directive on the first script in a chunk forces strict mode onto every sloppy-mode script bundled after it -- e.g. thickbox, which then crashes with `ReferenceError: imgLoader is not defined` on its implicit globals. Wrap each bundle in an IIFE so esbuild's `'use strict'` becomes the first statement inside the function (function level) instead of at file scope. esbuild's `globalName` assigns onto a locally-declared `var` root, which the wrapper would trap, so re-expose the package global on `window` in the footer. Ref: https://core.trac.wordpress.org/ticket/65515 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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. |
westonruter
added
[Type] Build Tooling
|
Size Change: +1.91 kB (+0.02%) Total Size: 7.78 MB 📦 View Changed
|
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
Rather than use banner for the header, would it be possible to introduce baseConfig.header as an equivalent to footer. My reading of the word banner is that it's intended for an inline comment /* This file is generated from....
Also needs to use multi-line comment format per coding standards https://developer.wordpress.org/coding-standards/inline-documentation-standards/javascript/#multi-line-comments
Is there actually a The documentation for And the documentation for So it sounds like they expect you to use these two options if you need to wrap the output. |
Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
Thanks @siliconforks, my mistake.
|
Flaky tests detected in a807a88. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/30682728569
|
|
@westonruter Thinking about it overnight, I think a changelog entry is warranted for this:
|
…ix/esbuild-file-level-use-strict-65515
westonruter
added
[Type] Bug
Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
…hub.com/WordPress/gutenberg into fix/esbuild-file-level-use-strict-65515
@peterwilsoncc thanks, added in bd1ba10 |
There was a problem hiding this comment.
Pull request overview
Wraps esbuild package bundles in IIFEs to prevent strict mode leaking into concatenated legacy scripts.
Changes:
- Adds function-scoped bundle wrappers and restores package globals on
window. - Documents the fix in the changelog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/wp-build/lib/build.mjs |
Adds IIFE wrapping and global re-exposure. |
packages/wp-build/CHANGELOG.md |
Records the strict-mode isolation fix. |
| * bundled after it, throwing on e.g. implicit globals. Wrapping confines the | ||
| * directive to this bundle. See https://core.trac.wordpress.org/ticket/65515. | ||
| */ | ||
| baseConfig.banner = { js: '(function() {' }; |
There was a problem hiding this comment.
🤖 Validation of this suggestion by Claude Code (Opus 5)
The mechanism Copilot describes is real, but it doesn't apply here, and the "High" severity
isn't warranted.
The premise is correct. wp-admin/load-scripts.php does concatenate raw file contents with
only a newline between them:
$out .= get_file( $path ) . "\n";And ASI does not insert a semicolon before ( — foo()\n(function(){})() parses as one call
chain. So the bug class is real.
But the trigger condition doesn't occur for any file that can precede these bundles:
-
The set of possible predecessors is closed and small.
load-scripts.phponly serves
handles registered bywp_default_scripts()/wp_default_packages_*(), and
WP_Scripts::do_item()only concatenates a script whenin_default_dir()passes
(default_dirs = array( '/wp-admin/js/', '/wp-includes/js/' )). Plugin and theme scripts are
never concatenated, so the only thing that can sit immediately before a bundle is a
core-shipped file. -
All of them were checked. Parsing all 849 core JS files under
wp-includes/js,
wp-admin/js, andjs/_enqueueswith acorn and comparing the top-level statement count of
srcagainstsrc + "\n(function() {})();"— if the leading(were absorbed, the count
wouldn't increase — yields zero absorbers, minified and unminified alike. -
The condition is also narrower than "omits its final semicolon." The preceding file's last
token has to be a valid callee: an identifier,),], a literal, or the}of an
object/function expression. A file ending in a function declaration's}, or ending in a
comment (as the sourcemap-annotated builds do), is already safe. -
Core has shipped this exact pattern for years. 15 concatenatable minified core scripts
already begin with a bare(and no defensive semicolon — including
js/dist/vendor/react.min.js,js/dist/vendor/react-dom.min.js,js/media-views.min.js,
js/codemirror/codemirror.min.js, andjs/jquery/jquery.hotkeys.min.js. If any preceding
core script could swallow a leading(, React would already be broken in the admin.
Separately, the footer here ends with })();, so this bundle can't break whatever is
concatenated after it either.
Adding the ; is still reasonable as free defense-in-depth — ;(function(){ is the
conventional concatenation-safety idiom, it costs one byte, and sourcemaps are unaffected since
the banner stays on one line. But it guards against a condition no core file currently meets,
and 15 existing core scripts already don't guard against it.
|
Just want to add that this makes sense to me. I don't see any other alternative. |
|
It's a pity that esbuild cannot do this automatically. The script content is already an IIFE whose result is assigned to the exporting global. Now we have a double IIFE. |
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
I tried to find out where the "use strict" statement comes in the first place. esbuild inserts it when the tsconfig.json file declares the "strict": true or "alwaysStrict": true option.
If we wanted to disable it, we could pass a custom tsconfig:
esbuild.build({
// ...
tsconfigRaw: { compilerOptions: { alwaysStrict: false } },
});
I think it would be quite safe to remove, but we probably don't want to make such a change. The old webpack build added it too, although there it was inside the generated IIFE and didn't leak outside.
I think that the leaking use strict is an esbuild bug that's worth reporting. Doing it here: evanw/esbuild#4505
| if ( packageJson.wpScriptDefaultExport ) { | ||
| footerJs += `if (typeof ${ globalName } === 'object' && ${ globalName }.default) { ${ globalName } = ${ globalName }.default; }\n`; | ||
| } | ||
| footerJs += `(window.${ scriptGlobal } ||= {}).${ globalMember } = ${ scriptGlobal }.${ globalMember };\n`; |
There was a problem hiding this comment.
The globalName variable now becomes redundant, and doesn't need to be passed to esbuild as the globalName option. With the additional IIFE wrapper, it's no longer a global, it's a local variable. It can have a constant name, e.g., foo. Then the esbuild option is globalName: 'foo', and the footer assignment code is like:
`(window.${scriptGlobal} ||= {}).${globalMember} = foo;`
`(window.${scriptGlobal} ||= {}).${globalMember} = foo.default;`
|
I'm going ahead with the merge so that this is part of RC1, as there have been 3 approvers (1 in comment only). |
|
@t-hamano We need to add the |
Let's backport this to 7.1 and include it in RC1. |
t-hamano
added
the
Backport to WP 7.1 Beta/RC
github-actions
Bot
removed
the
Backport to WP 7.1 Beta/RC
* Build: Wrap script bundles in an IIFE to contain 'use strict' esbuild's `format: 'iife'` emits a `'use strict'` directive at the top of each package bundle. Because WordPress concatenates registered scripts by joining raw file contents (wp-admin/load-scripts.php), a file-level directive on the first script in a chunk forces strict mode onto every sloppy-mode script bundled after it -- e.g. thickbox, which then crashes with `ReferenceError: imgLoader is not defined` on its implicit globals. Wrap each bundle in an IIFE so esbuild's `'use strict'` becomes the first statement inside the function (function level) instead of at file scope. esbuild's `globalName` assigns onto a locally-declared `var` root, which the wrapper would trap, so re-expose the package global on `window` in the footer. Ref: https://core.trac.wordpress.org/ticket/65515 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Use multi-line comment format Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org> * Add changelog entry Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org> --------- Co-authored-by: westonruter <westonruter@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: siliconforks <siliconforks@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
github-actions
Bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/7.1 branch to get it included in the next release: 14eff27 |
This updates the pinned commit hash of the Gutenberg repository from `fd715a6833679d098d9fee84b642f8f1bc27341b` to `f05e40e91c54f29c449b1f33d0db89f5166812d9`. A full list of changes included in this commit can be found on GitHub: WordPress/gutenberg@fd715a6...f05e40e - Writing flow: forward delete an empty paragraph without breaking apart the next block (WordPress/gutenberg#80813) - Upload Media: Fail the item when the /finalize request fails (WordPress/gutenberg#80725) - Fix template `modified` and `date` return value for file templates (WordPress/gutenberg#80733) - Boot: Adjust specificity of the image reset styles so components can size their own images (WordPress/gutenberg#80845) - Quote: Ensure paragraph placeholder appears after deleting nested blocks (WordPress/gutenberg#77151) - Block editor: make the Group action wrap blocks with a group transform (WordPress/gutenberg#80891) - Copy: preserve the block when its entire text is selected (WordPress/gutenberg#80994) - Add opt-out for block style state controls (WordPress/gutenberg#80956) (WordPress/gutenberg#81004) - Tabs: Support Home and End keys for keyboard navigation (WordPress/gutenberg#80912) - Rename blockStatesEnabled setting to blockStatesEditingEnabled (WordPress/gutenberg#81058) - [WP 7.1] Background: Fix the legacy gradient UI where a gradient cannot be selected (WordPress/gutenberg#81059) - Views: honor developer-defined view config overrides (WordPress/gutenberg#80832) - Playlist: Add track icon (WordPress/gutenberg#81078) - Remove the CODEOWNERS file from wp/7.1. (WordPress/gutenberg#81104) - Notes: Email users mentioned in a note (WordPress/gutenberg#79606) - Backport 81068 80744 80642 (
What?
Wrap each package script bundle produced by
@wordpress/build(esbuild) in an IIFE so the'use strict'directive esbuild emits ends up at the function level instead of the file level.Why?
esbuild's
format: 'iife'output currently begins with a file-level'use strict';directive, e.g.wp-includes/js/dist/hooks.js:WordPress concatenates registered admin scripts by joining raw file contents (
wp-admin/load-scripts.phpdoes$out .= get_file( $path ) . "\n";— no per-script wrapping). A'use strict'directive at the very top of the first script in a concatenated chunk sits in the combined script's directive prologue, which forces every script after it in that chunk into strict mode.Many legacy core scripts are written for sloppy mode and rely on things strict mode forbids (implicit globals, etc.). The reported symptom is the ThickBox modal on the Plugins screen: the bundle
wp-hooks,jquery-core,jquery-migrate,thickboxputswp-hooks(strict) first, forcingthickbox.jsinto strict mode, where its implicit globalimgLoader = new Image()throws:This regressed when packages migrated to esbuild in #72125. Trac: https://core.trac.wordpress.org/ticket/65515
How?
In
packages/wp-build/lib/build.mjs, wrap the bundle:banner: '(function() {'— esbuild's'use strict'now lands as the first statement inside the function (function-level), so it can no longer sit in a concatenated file's prologue.globalNameassigns onto a locally-declaredvarroot (and awindow.…global name would emitvar window, shadowing the real global), that assignment would be trapped inside the wrapper — so the footer re-exposes the package global onwindow.Resulting output:
Sourcemaps remain valid (esbuild accounts for
banner/footerline counts), unlike a post-build string transform.Testing Instructions
npm run buildand confirm no bundle inbuild/scripts/*/index.jsbegins with"use strict"— each now starts with(function() {.window.wp.*globals (e.g.wp.hooks,wp.i18n,wp.element) are populated.CONCATENATE_SCRIPTSon,SCRIPT_DEBUGoff), open a plugin's "View details" ThickBox modal on the Plugins screen and confirm it opens without a consoleReferenceError.🤖 Generated with Claude Code