Add wp-build-polyfills package for WP < 7.0 compatibility#47367
Add wp-build-polyfills package for WP < 7.0 compatibility#47367
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new wp-build-polyfills package to provide WordPress Core packages (@wordpress/private-apis, @wordpress/theme, @wordpress/boot, @wordpress/route, @wordpress/a11y) as polyfills for WordPress versions prior to 7.0. The package includes a custom esbuild-based build script that bundles these packages as both classic scripts (IIFE format) and script modules (ESM format), with externals handling that matches wp-build's strategy. The polyfills are conditionally registered only when not already provided by Core or Gutenberg, and are integrated into the Forms package as a build dependency.
Changes:
- Created new
wp-build-polyfillspackage with build script, PHP registration class, and package configuration - Integrated polyfills into Forms package by adding dependency and calling registration during dashboard initialization
- Updated build scripts to generate polyfills before running wp-build
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/wp-build-polyfills/src/class-wp-build-polyfills.php | PHP class for conditional polyfill registration |
| projects/packages/wp-build-polyfills/package.json | Package configuration with devDependencies and build script |
| projects/packages/wp-build-polyfills/composer.json | Composer configuration for PHP package |
| projects/packages/wp-build-polyfills/bin/build-polyfills.mjs | Custom esbuild script to bundle polyfills with proper externals |
| projects/packages/wp-build-polyfills/.gitignore | Ignore node_modules |
| projects/packages/wp-build-polyfills/changelog/fix-polyfill-wp-build-dependencies | Changelog entry for new package |
| projects/packages/forms/src/dashboard/class-dashboard.php | Integration of polyfill registration in Forms dashboard |
| projects/packages/forms/package.json | Added build-polyfills script and dependency |
| projects/packages/forms/composer.json | Added wp-build-polyfills as PHP dependency |
| projects/packages/forms/changelog/fix-polyfill-wp-build-dependencies | Changelog entry for Forms integration |
| pnpm-lock.yaml | Lockfile updates for new dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
projects/packages/wp-build-polyfills/src/class-wp-build-polyfills.php:26
- The documentation for the
$base_fileparameter should clarify that it expects a reference to a PHP file within the plugin (typically__FILE__), not a constructed path. This would help prevent the misuse seen in the Forms integration. Consider updating the parameter description to: "File path for plugins_url() computation. Should be FILE or a reference to a PHP file in the plugin."
* @param string $base_file File path for plugins_url() computation.
projects/packages/forms/changelog/fix-polyfill-wp-build-dependencies:2
- The changelog type is listed as "fixed" but this change is adding a new feature (polyfill support) rather than fixing an existing bug. Consider using "added" or "changed" as the type instead to more accurately reflect the nature of the change.
Type: fixed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $forms_root = dirname( __DIR__, 2 ); | ||
| \Automattic\Jetpack\WP_Build_Polyfills\WP_Build_Polyfills::register( | ||
| $forms_root, | ||
| $forms_root . '/build/polyfills/modules/boot/index.min.js' |
There was a problem hiding this comment.
The second parameter to WP_Build_Polyfills::register() should be __FILE__ or a similar reference to the current PHP file, not a constructed path to a JavaScript file. The $base_file parameter is used with plugins_url() which expects a file path that's part of the plugin to compute the plugin's base URL. The constructed path $forms_root . '/build/polyfills/modules/boot/index.min.js' is incorrect and won't compute the correct URL.
According to the convention seen throughout the codebase, this should be __FILE__ so that plugins_url() can properly determine the plugin base directory and construct the correct URL.
| $forms_root . '/build/polyfills/modules/boot/index.min.js' | |
| __FILE__ |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
github-actions
bot
added
the
[Plugin] Jetpack
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 17 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_action( | ||
| 'wp_default_scripts', | ||
| function () use ( $polyfills_dir, $base_file ) { | ||
| self::register_modules( $polyfills_dir, $base_file ); | ||
| }, | ||
| 20 | ||
| ); |
There was a problem hiding this comment.
The second add_action( 'wp_default_scripts', … ) callback is declared with no parameters, but wp_default_scripts passes the WP_Scripts instance as an argument. With the default accepted_args of 1, this will be invoked with 1 argument and can trigger an ArgumentCountError on PHP 8+. Fix by either accepting the $scripts parameter in the closure (even if unused) or by passing 0 as the accepted_args argument to add_action for this callback.
| wp_register_script_module( | ||
| $module_id, | ||
| plugins_url( 'build/polyfills/modules/' . $name . '/index.min.js', $base_file ), | ||
| $asset['module_dependencies'] ?? array(), |
There was a problem hiding this comment.
register_modules() passes only $asset['module_dependencies'] into wp_register_script_module(), but the generated asset file also contains a standard 'dependencies' list (e.g. classic script handles like wp-i18n, jquery, etc). Script modules in this codebase are enqueued with mixed dependency arrays (module IDs + classic script handles), so dropping 'dependencies' here can cause required classic scripts to not be loaded before the module executes. Consider merging $asset['dependencies'] and $asset['module_dependencies'] (or emitting a single dependencies array from the build step) and passing the combined list to wp_register_script_module().
| wp_register_script_module( | |
| $module_id, | |
| plugins_url( 'build/polyfills/modules/' . $name . '/index.min.js', $base_file ), | |
| $asset['module_dependencies'] ?? array(), | |
| $dependencies = array_unique( | |
| array_merge( | |
| $asset['dependencies'] ?? array(), | |
| $asset['module_dependencies'] ?? array() | |
| ) | |
| ); | |
| wp_register_script_module( | |
| $module_id, | |
| plugins_url( 'build/polyfills/modules/' . $name . '/index.min.js', $base_file ), | |
| $dependencies, |
The bin entry in package.json handles execution permissions via npm/pnpm — no need for the git executable bit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The bin/build-polyfills.mjs script is a Node.js dev build tool and should not be shipped in the jetpack_vendor directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| "build:dashboard": "webpack --config ./tools/webpack.config.dashboard.js", | ||
| "build:form-editor": "webpack --config ./tools/webpack.config.form-editor.js", | ||
| "build:wp-build": "wp-build", | ||
| "build:polyfills": "build-polyfills", |
There was a problem hiding this comment.
This is not a good code pattern for the monorepo. jetpack build should already handle ordering of the builds to make sure the depended-on package is built before the dependent package.
Adding this here, and calling it from build:wp-build which is called from build which is called from the build-production and build-development scripts used by jetpack build, will result in the polyfill package getting built twice. Or it will once you set up the package correctly.
... Oh, you're building the scripts into this package, instead of having the wp-build-polyfills package contain the scripts as well as the PHP code to register them? That seems incredibly strange to me, why are you doing it that way?
There was a problem hiding this comment.
I asked that "it needs to to be a separate package, but the output must be used by Forms", so Claude came up with this solution. I'm testing a refactor to completely avoid this, but I rebased Gutenberg and now there is another bug there that needs to be tracked before I can verify Jetpack's side and update this PR. Will ping you again when possible.
| import { build } from 'esbuild'; | ||
|
|
||
| // Resolve packages from this package's own node_modules, not the consumer's. | ||
| const __dirname = path.dirname( fileURLToPath( import.meta.url ) ); |
There was a problem hiding this comment.
I recently discovered that import.meta.dirname exists.
| const __dirname = path.dirname( fileURLToPath( import.meta.url ) ); | |
| const __dirname = import.meta.dirname; |
|
|
||
| // Resolve packages from this package's own node_modules, not the consumer's. | ||
| const __dirname = path.dirname( fileURLToPath( import.meta.url ) ); | ||
| const packageRoot = path.resolve( __dirname, '..' ); |
There was a problem hiding this comment.
Is there and advantage to this over
| const packageRoot = path.resolve( __dirname, '..' ); | |
| const packageRoot = path.dirname( __dirname ); |
?
There was a problem hiding this comment.
I'm wary that you seem to be trying to reinvent Webpack with this script. But you're missing a bunch of the stuff that our actual Webpack config does, like make sure i18n will still work with translate.wordpress.org.
| } ) | ||
| ); | ||
|
|
||
| // Non-minified |
There was a problem hiding this comment.
Seems like a waste of space to build both a minified version with source maps and an unminified version (still with source maps).
Our normal setup does minified versus unminified based on whether it's run via jetpack build or jetpack build --production, and the latter is what gets published.
| } ) | ||
| ); | ||
|
|
||
| // Non-minified |
| add_action( | ||
| 'wp_default_scripts', | ||
| function ( $scripts ) use ( $polyfills_dir, $base_file ) { | ||
| self::register_scripts( $scripts, $polyfills_dir, $base_file ); | ||
| }, | ||
| 20 | ||
| ); | ||
|
|
||
| add_action( | ||
| 'wp_default_scripts', | ||
| function () use ( $polyfills_dir, $base_file ) { | ||
| self::register_modules( $polyfills_dir, $base_file ); | ||
| }, | ||
| 20 | ||
| ); |
There was a problem hiding this comment.
Why are these two separate functions for the same hook?
| ), | ||
| 'wp-theme' => array( | ||
| 'path' => 'theme', | ||
| 'deps' => array( 'wp-element', 'wp-private-apis' ), |
There was a problem hiding this comment.
Having deps here seems pointless. You don't register it at all if there's no .asset.php file, and that file should always contain dependencies.
| }; | ||
| } ); | ||
|
|
||
| // Generate asset file on build end |
There was a problem hiding this comment.
Wow, you're even reinventing this from scratch.
| 'wp-private-apis' => array( | ||
| 'path' => 'private-apis', | ||
| 'deps' => array(), | ||
| // Always replace: older Core versions ship private-apis with an |
There was a problem hiding this comment.
What if we're running in a newer Core version, though?
Proposed changes:
wp-build-polyfillspackage that bundles WordPress Core packages (@wordpress/private-apis,@wordpress/theme,@wordpress/boot,@wordpress/route,@wordpress/a11y) as polyfills for WordPress < 7.0build-polyfills) using esbuild that produces both classic scripts (IIFE) and script modules (ESM) with proper externals handling matching wp-build's strategyWP_Build_Polyfillsconditionally registers the polyfill scripts/modules only when they are not already provided by Core or GutenbergOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
pnpm installthenpnpm jetpack build packages/formsprojects/packages/forms/build/polyfills/is generated with bothscripts/andmodules/subdirectories containing bundled polyfills and.asset.phpfilesChangelog