wp-build-polyfills: add export-contract build check to catch version-skew - #50676
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code Coverage Summary2 files are newly checked for coverage.
|
| // resolved via the browser import map rather than a window global, so they | ||
| // have a different (import-map) failure mode. Verifying them is a documented | ||
| // follow-up; see README "Export-contract validation". | ||
| const PROVIDER_PACKAGES = [ |
There was a problem hiding this comment.
Lets just have one source of truth for this list. So that we don't have to keep it updated in two places.
The same goes for @wordpress/boot', '@wordpress/route', '@wordpress/a11y'
There was a problem hiding this comment.
I agree with Enej here. The list comes from https://github.com/Automattic/jetpack/blob/trunk/projects/packages/wp-build-polyfills/src/class-wp-build-polyfills.php#L22. It needs some mapping to js package names.
dhasilva
left a comment
There was a problem hiding this comment.
Initial review — no blockers. Additive, build-time-only guard. I ran the suite (45/45 green) and probed the parser against the installed @wordpress/* tree; it's correct for the ESM shapes it actually runs against, and the simulate-skew test genuinely proves the boot → theme pair is scanned end-to-end (not just "green because it matched nothing").
Duplicated package lists — agree with the single-source-of-truth point already raised
PROVIDER_PACKAGES / CONSUMER_PACKAGES in validate-export-contract-lib.js duplicate a list that already exists in src/class-wp-build-polyfills.php (SCRIPT_HANDLES, L22) and in webpack.config.js. The PR keeps them honest with a drift test (PROVIDER_PACKAGES matches webpack classicPolyfills / CONSUMER_PACKAGES matches webpack modulePolyfills), but that guards around the duplication rather than removing it.
This is the same concern @enejb and @dhasilva raised inline:
+1 — deriving both lists from one source (with the PHP→JS package-name mapping dhasilva mentions) is preferable, and it would also dissolve suggestion #2 below.
Robustness suggestions (all non-blocking)
-
parsePublicExportsonly recognizes consolidatedexport { … }blocks. It misses inlineexport const/function/class(verified:export const ThemeProvider = …→names: []). All four current providers emit consolidated blocks, so it's green today — but a future@wordpress/*bump that ships an inline-exportindex would make the check report a legitimately-exported symbol as missing and spuriously fail the build. Cheap to harden by also matchingexport\s+(?:const|function|class|let|var)\s+(\w+). -
Opaque (
export *) and unresolved providers silently drop coverage. The lib tracksskipped/errors, but the CLI prints nothing on success and only throws on!ok. So if a provider index switches toexport * from './x', its contract is silently skipped and the build stays green — the exact silent failure class this guard exists to prevent. Aconsole.warnper skipped/opaque/unresolved entry would keep coverage regressions visible. -
parseNamedImportsdoesn't distinguish commented-out or re-export lines. It treats// import { X } from '@wordpress/theme'as a real import (false positive) and ignoresexport { X } from …re-exports and mixedimport Default, { X }(false negatives). None of these shapes occur in the currentboot/route/a11ybuild output, so low real-world risk — a one-line comment documenting the "clean named-import lines only" assumption would suffice.
Scope
This is a name-presence contract, not a semantic/version check: a symbol that is present but broken (wrong signature, stub implementation) passes. That's an appropriate boundary for a static build-time guard and the PR is honest about it — noting it only so the guarantee isn't over-read.
Verdict: minor issues — can merge after addressing. Also resolve the draft's P2/Slack/Linear TODO before marking ready for review.
Generated by Claude.
Roadmap: making
|
Status update on the follow-upsChecked where the other layers from the roadmap actually stand — most are already covered:
Still open (feedback welcome):
|
anomiex
left a comment
There was a problem hiding this comment.
At a quick glance, it looks like your AI went way overboard with the comments and readme docs, and possibly over-engineered the testing too. Does it really need a CLI command run during the build and the same code run in the unit testing?
Also, one other thing in progress that your AI missed: #50621
…skew blank dashboards
Ship a build-time guard that fails the build when a polyfilled package imports
a symbol the shipped version of another polyfilled package does not export —
the Jetpack 16.0 blank-dashboard failure mode (boot imported ThemeProvider but
the shipped @wordpress/theme only exposed it privately → undefined at runtime).
- bin/validate-export-contract{,-lib}.js: scan consumer imports (boot/route/a11y)
vs classic-script provider exports (theme/notices/private-apis/views) from the
packages' published ESM source; fail on any missing symbol.
- Wire into the build script so a violation reddens the build + test-js CI jobs.
- Manual triggers: `check-contracts`, `simulate:skew` (WP_BUILD_POLYFILLS_SIMULATE_MISSING),
plus a real-version-pin recipe in the README.
- tests/js/validate-export-contract.test.js: 19 cases incl. the 16.0 regression,
real-tree green check, simulated-skew red check, and webpack-config drift test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…test coverage - package.json: sort scripts lexicographically (package-json/sort-collections). - Cover the CLI wrapper end-to-end (child_process) and defensive lib branches (parseSimulateEnv malformed input, formatError errors path, unresolvable packages), matching the existing validate-boot-asset-lib coverage bar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e of truth Address review (enejb, dhasilva): don't maintain the shipped-package list in a second place. Derive the export-contract validator's provider/consumer lists from SCRIPT_HANDLES + MODULE_IDS in class-wp-build-polyfills.php (mapping wp-* handles → @wordpress/* names), the same constants that register them at runtime. The drift test now guards PHP↔webpack agreement instead of comparing to a hardcoded copy, so a polyfill added to one but not the other fails the build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (anomiex): trim the AI-verbose comments and README to match the codebase, and cut the over-engineered test surface. - Concise comments/JSDoc; README safety note reduced to a short bullet list. - Remove the WP_BUILD_POLYFILLS_SIMULATE_MISSING env feature and the `simulate:skew` script (manual-trigger meta-tooling); keep the `simulateMissing` test hook that drives the 16.0 regression test. - Tests: 27 → 7 (16.0 regression, import/export parsing, PHP-derivation↔webpack sync, real-tree green + simulated-skew, CLI green). Same behavior: the check still runs in `build` and trips on a missing export. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cd05eea to
d9b1a61
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new post-build “export contract” validator to wp-build-polyfills to catch @wordpress/* version-skew where a consumer imports a symbol that the shipped provider package does not export (the Jetpack 16.0 blank-dashboard failure mode). It complements the existing .asset.php dependency-handle validation by checking the actual JS export surface area.
Changes:
- Add
bin/validate-export-contract.js+ shared library to verify consumer named imports exist in provider public exports. - Wire the new validator into
pnpm run build, and add a standalonepnpm run check-contractsscript. - Add Node test coverage for parsing/contract checking + installed-tree validation, plus README + changelog updates.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/wp-build-polyfills/bin/validate-export-contract.js | New CLI entrypoint that runs the export-contract validator post-build. |
| projects/packages/wp-build-polyfills/bin/validate-export-contract-lib.js | Core implementation: reads shipped provider/consumer lists, parses imports/exports, and reports contract violations. |
| projects/packages/wp-build-polyfills/tests/js/validate-export-contract.test.js | Regression and behavior tests covering parsing, contract failures, and installed-tree checks. |
| projects/packages/wp-build-polyfills/package.json | Wires export-contract validation into build and adds check-contracts. |
| projects/packages/wp-build-polyfills/README.md | Documents the new safety check and how to run it. |
| projects/packages/wp-build-polyfills/changelog/add-export-contract-validation | Changelog entry for the new build-time guard. |
- parseNamedImports: handle mixed default+named imports (`import Def, { Bar }`),
which the regex previously missed (silent false-negative).
- Warn (not silently skip) when a provider's index uses `export *` and can't be
statically verified — a barrel would otherwise open a hole in the guard.
- Cover the CLI failure path end-to-end: a test-only env hook injects a skew and
asserts the CLI exits non-zero (the actual CI gate).
- README: use full `@wordpress/*` package names in the provider list.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Took another pass over the post-feedback state ( I ran things locally to make sure the green isn't vacuous:
No blockers from me. A few small suggestions, all optional — the first two are the items from the earlier review round that I think are still worth closing, since both protect against the check quietly misbehaving: 1. Verified: for ( const m of indexSource.matchAll( /export\s+(?:const|function|class|let|var)\s+([\w$]+)/g ) ) {
names.add( m[ 1 ] );
}2. A few The To be fair, the simulated-skew test would catch that for 3.
4. Tiny docs nit — a one-line comment on 5. The description still has the Nice piece of work — the CLI red-path test asserting a non-zero exit rather than just the library return value is a good touch, since that's the actual CI gate. Generated by Claude. |
Address dhasilva's follow-ups (all closing "silent green" holes):
- parsePublicExports also reads inline `export const/function/...` declarations,
not just consolidated `export { … }` blocks — an inline-export bump would
otherwise report every symbol missing and fail a valid build.
- Warn (not silently continue) when a provider/consumer is unresolvable or a
consumer has no build-module/, and assert results.length > 0 in the real-tree
test so a vacuous green can't slip through.
- readPackageExports honors the `exports` map (exports['.'].import ?? module ?? main).
- Document parseNamedImports' import-only boundary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Proposed changes
wp-build-polyfillsships a matched set of@wordpress/*packages that call each other's exports at runtime (e.g.@wordpress/bootimportsThemeProviderfrom@wordpress/theme,SnackbarNoticesfrom@wordpress/notices). When those versions drift out of sync, an imported symbol resolves toundefinedat runtime — a blank dashboard with no build error and no obvious console message. That is the class of failure that shipped in Jetpack 16.0 (fixed in the 16.0.1 point release via #50515 / #50465).The existing post-build check (
validate-boot-asset.js) verifies dependency handle names, which is a different failure class and did not catch 16.0. This PR adds a second, complementary build-time guard that catches the export-skew class directly:bin/validate-export-contract.js(+-lib.js) — afterwebpack, for every symbol a consumer package (@wordpress/boot, …) imports from a polyfilled provider (@wordpress/theme,@wordpress/notices,@wordpress/private-apis,@wordpress/views), it asserts the provider's shipped public API actually exports it. Both sides are read from the packages' published ESM source (build-module/*.mjs), which is stable across builds/minification — not from the webpack bundle. The provider/consumer lists are derived fromSCRIPT_HANDLES/MODULE_IDSinclass-wp-build-polyfills.php(single source of truth). Scoped in v1 to the classic-script providers (thewindow.wp.<pkg>globals, where a missing export silently becomesundefined); the ESM module providers (route,a11y) are a documented follow-up.buildscript, so a violation failspnpm build→ fails thebuildandtest-jsCI jobs (same gate as the existing check). Run standalone withpnpm run check-contracts.tests/js/validate-export-contract.test.jscovers the exact 16.0 shape (a consumer imports a symbol the provider no longer exports), a real-installed-tree green check, a simulated-skew red check that asserts the CLI exits non-zero, import/export parsing, and a drift test keeping the derived package lists in sync withwebpack.config.js.No change to the polyfill's runtime behavior or public PHP/JS API — this is a build-time safety net only.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
From
projects/packages/wp-build-polyfills:package.json→"@wordpress/theme": "0.15.1"(a version that only exposedThemeProviderprivately — the real 16.0 cause), thenpnpm install && pnpm run build→ the build fails atvalidate-export-contract.js. Revert the pin +pnpm installwhen done. (The same red path is also asserted in the test suite via the “CLI exits non-zero … on a skew” case.)