Repository navigation
Conversation
The Gutenberg-nightly e2e job started failing on `can create new post`
because `getByLabel(/Add title|Block: Title/)` no longer matches the
title field in recent Gutenberg builds. The element is still exposed
as a textbox with the accessible name "Add title", so target it via
`getByRole('textbox', ...)` to keep working on older Gutenberg
versions (which expose the same accessible name through aria-label)
as well as on trunk.
Gutenberg's new esbuild-based bundling pipeline (WordPress/gutenberg
PR #72140) appends `wp.X = Object.assign({}, wp.X)` to every
`window.wp.*` global to materialize getter-based exports. Object.assign
only copies enumerable own properties, so the non-enumerable
`__esModule: true` flag is dropped — which breaks webpack's
default-import interop (`__webpack_require__.n` falls back to returning
the whole namespace object instead of `.default`). The first symptom
is the Cart block crashing with "isShallowEqual is not a function" on
sites running Gutenberg trunk.
Resolve isShallowEqual through the namespace so the function is picked
up whether the runtime exposes it as `default` (WordPress core's
existing build) or as a named export (Gutenberg trunk's new build).
`getCanvas` in the e2e-utils-playwright package checks for the `iframe[name="editor-canvas"]` element synchronously, so on recent Gutenberg builds — where the iframe is mounted slightly after initial paint — the canvas falls back to the outer page. The title locator then waits forever against the wrong frame and times out at 20s. Add a short, soft-failing wait for the iframe so older Gutenberg releases that render the title directly in the page DOM keep working.
Testing GuidelinesHi @senadir @ObliviousHarmony , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
|
This is only impacted by nightly Gutenberg, best I know, but assigning it to 10.8.0 so we can get it out to sites before Gutenberg ships it. |
The previous commit replaced the `is-shallow-equal` default imports with relative-path imports of the local shim, but left them in the "External dependencies" comment group. Re-group them under "Internal dependencies" to satisfy the woocommerce/dependency-group ESLint rule (a couple of files needed a new Internal block as they didn't have one yet).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCreates a local isShallowEqual adapter, updates many imports to use it, adjusts a filter-registry import pattern, hardens an editor e2e title locator, and adds two changelog entries. ChangesGutenberg isShallowEqual namespace resolution and e2e test fixes
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/woocommerce/client/blocks/assets/js/utils/is-shallow-equal.ts`:
- Around line 23-26: The export resolution for isShallowEqual may pick
non-function values and later crash; update the assignment to pick the first
candidate that is a callable function by checking typeof === 'function' on
moduleExports.default, moduleExports.isShallowEqual, and IsShallowEqualModule
(and fallback) before assigning to isShallowEqual; reference the symbols
isShallowEqual, moduleExports, IsShallowEqualModule, and IsShallowEqualFn and
implement the guard so only a function-valued candidate is used (otherwise throw
a clear error or provide a safe noop).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9e4bf724-1526-4c65-be1d-8b3987ad5ed9
📒 Files selected for processing (17)
plugins/woocommerce/changelog/e2e-fix-gutenberg-nightly-title-locatorplugins/woocommerce/changelog/fix-gutenberg-nightly-is-shallow-equal-default-importplugins/woocommerce/client/blocks/assets/js/base/components/cart-checkout/form/form.tsxplugins/woocommerce/client/blocks/assets/js/base/components/cart-checkout/form/validate-state.tsplugins/woocommerce/client/blocks/assets/js/base/context/hooks/shipping/use-shipping-data.tsplugins/woocommerce/client/blocks/assets/js/base/context/hooks/use-query-state.jsplugins/woocommerce/client/blocks/assets/js/base/hocs/with-reviews.tsxplugins/woocommerce/client/blocks/assets/js/base/hooks/use-shallow-equal.tsplugins/woocommerce/client/blocks/assets/js/blocks/attribute-filter/block.tsxplugins/woocommerce/client/blocks/assets/js/blocks/rating-filter/block.tsxplugins/woocommerce/client/blocks/assets/js/blocks/stock-filter/block.tsxplugins/woocommerce/client/blocks/assets/js/data/cart/push-changes.tsplugins/woocommerce/client/blocks/assets/js/data/validation/reducers.tsplugins/woocommerce/client/blocks/assets/js/hocs/with-product-variations.tsxplugins/woocommerce/client/blocks/assets/js/utils/is-shallow-equal.tsplugins/woocommerce/client/blocks/packages/checkout/filter-registry/index.tsplugins/woocommerce/tests/e2e-pw/utils/editor.ts
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a commit that changes plugin code. |
|
Size Change: +4.78 kB (+0.08%) Total Size: 5.76 MB
|
Walk the candidate exports and pick the first one that's actually a function, rather than relying on `??` and a final type cast. If no candidate is callable (a state we shouldn't be able to reach with either the current npm `is-shallow-equal` package or the trunk Gutenberg build), throw a clear error at import time so the failure is loud and traceable instead of a delayed "X is not a function". Also drop the placeholder issue number in the explanatory comment in favor of the actual upstream report.
|
Technically @kraftbj! To be honest, having to deeply import a shim like this for any affected Essentially, we would add a runtime helper that restores the // Consumer-side guard that restores the non-enumerable `__esModule` flag when it's
// missing on an externalized `window.wp.*` module. Gutenberg's esbuild build pipeline
// (WordPress/gutenberg#78697) appends `wp.X = Object.assign({}, wp.X)` to every
// `window.wp.*` global to flatten getter-based exports; `Object.assign` only copies
// enumerable own properties, so the non-enumerable `__esModule` marker is dropped.
// Webpack's default-import interop helper (`__webpack_require__.n`) keys off that
// marker — without it, default imports of any `@wordpress/*` package with a default
// export resolve to the namespace object instead of `.default` and crash at the call
// site. We restore the flag in place on the same object Gutenberg shipped, which is
// equivalent to undoing the strip that the upstream pipeline introduced. Runtime is
// a no-op when the flag is already present, when the module has no `default`
// property, or when the module isn't an object.
const RESTORE_ESM_HELPER = `(function (m) {
if (m && typeof m === 'object' && 'default' in m && !m.__esModule) {
Object.defineProperty(m, '__esModule', { value: true });
}
return m;
})`;With a helper like this: /**
* Build the JS expression that materializes a `window`-style external for the
* non-module ('var' externalsType) path. For `@wordpress/*` requests the
* expression is wrapped with RESTORE_ESM_HELPER to restore the `__esModule`
* flag stripped by Gutenberg's esbuild pipeline; for everything else the
* expression is the byte-equivalent property-walk that externalsType 'window'
* would have produced (`window["seg1"]["seg2"]...`).
*
* Returns `undefined` when the request shouldn't be externalized, mirroring
* the upstream callback contract.
*
* @param {string} request The original import string (e.g. `'@wordpress/is-shallow-equal'`).
* @param {string | string[] | undefined} result The value resolved by `externalizeWpDeps`.
* @return {string | undefined} The JS expression to emit, or `undefined` if not externalized.
*/
const buildExternalExpression = ( request, result ) => {
if ( result === undefined ) {
return undefined;
}
const segments = Array.isArray( result ) ? result : [ result ];
const expression =
'window' +
segments
.map( ( segment ) => `[${ JSON.stringify( segment ) }]` )
.join( '' );
if ( request.startsWith( '@wordpress/' ) ) {
return `${ RESTORE_ESM_HELPER }(${ expression })`;
}
return expression;
};We could expand if ( this.useModules ) {
// Script-modules output: externals resolve to ESM specifiers and consumers use direct
// binding (not `__webpack_require__.n`), so the Gutenberg `__esModule`-strip regression
// doesn't apply. Pass through to the parent's behavior unmodified.
this.externalsPlugin = new compiler.webpack.ExternalsPlugin(
'import',
this.externalizeWpDeps.bind( this )
);
} else {
// Script (non-module) output: externals materialize as `window.wp.*` reads and consumers
// route default imports through `__webpack_require__.n`, which keys off `__esModule`. We
// switch externalsType from 'window' to 'var' so we can wrap each `@wordpress/*` external
// with RESTORE_ESM_HELPER. See the helper definition above for the upstream Gutenberg
// regression this works around. Non-`@wordpress/*` externals emit the byte-identical
// `window[seg]...` access they would have under externalsType 'window'.
this.externalsPlugin = new compiler.webpack.ExternalsPlugin(
'var',
( data, externalsCallback ) => {
this.externalizeWpDeps( data, ( err, result ) => {
if ( err ) {
return externalsCallback( err );
}
externalsCallback(
null,
buildExternalExpression( data.request, result )
);
} );
}
);
}It adds a few KB to the output size but if we're concerned about that we could emit it into the webpack runtime via a RuntimeModule. I tested this approach by adding the following test to the plugin: const { __test__ } = require( '../index' );
const { buildExternalExpression, RESTORE_ESM_HELPER } = __test__;
describe( 'buildExternalExpression', () => {
describe( '@wordpress/* externals (wrapped with RESTORE_ESM_HELPER)', () => {
it( 'wraps a two-segment @wordpress/* path', () => {
expect(
buildExternalExpression( '@wordpress/is-shallow-equal', [
'wp',
'isShallowEqual',
] )
).toBe(
`${ RESTORE_ESM_HELPER }(window["wp"]["isShallowEqual"])`
);
} );
it( 'wraps a single-segment @wordpress/* result', () => {
expect(
buildExternalExpression( '@wordpress/foo', 'wpFoo' )
).toBe( `${ RESTORE_ESM_HELPER }(window["wpFoo"])` );
} );
it( 'each wrap begins with the same RESTORE_ESM_HELPER prefix', () => {
const wrapped = buildExternalExpression(
'@wordpress/is-shallow-equal',
[ 'wp', 'isShallowEqual' ]
);
expect( wrapped.startsWith( RESTORE_ESM_HELPER + '(' ) ).toBe(
true
);
} );
} );
describe( 'non-@wordpress externals (byte-equivalent to externalsType "window")', () => {
it( 'emits a plain window[seg1][seg2] walk for @woocommerce/* paths', () => {
expect(
buildExternalExpression( '@woocommerce/blocks-checkout', [
'wc',
'blocksCheckout',
] )
).toBe( 'window["wc"]["blocksCheckout"]' );
} );
it( 'emits a single property access for single-string results', () => {
expect( buildExternalExpression( 'jquery', 'jQuery' ) ).toBe(
'window["jQuery"]'
);
} );
it( 'JSON-escapes segments containing special characters', () => {
expect(
buildExternalExpression( 'weird', [ 'a', 'b"c', 'd\\e' ] )
).toBe( 'window["a"]["b\\"c"]["d\\\\e"]' );
} );
it( 'does not wrap requests in other @-namespaces', () => {
expect(
buildExternalExpression( '@scoped/some-pkg', [ 'X', 'Y' ] )
).toBe( 'window["X"]["Y"]' );
} );
} );
describe( 'non-externalized requests', () => {
it( 'returns undefined when result is undefined', () => {
expect(
buildExternalExpression( '@wordpress/is-shallow-equal', undefined )
).toBeUndefined();
expect(
buildExternalExpression( 'unrelated-package', undefined )
).toBeUndefined();
} );
} );
} );
describe( 'RESTORE_ESM_HELPER (runtime semantics)', () => {
// Construct the actual function from the emitted string so we can exercise
// it directly. This is what the bundle's external module would do at runtime.
const helper = eval( RESTORE_ESM_HELPER ); // eslint-disable-line no-eval
it( 'is a no-op when __esModule is already true', () => {
const moduleObj = Object.defineProperty(
{ default: () => 'ok' },
'__esModule',
{ value: true }
);
const result = helper( moduleObj );
expect( result ).toBe( moduleObj );
expect( result.__esModule ).toBe( true );
} );
it( 'restores __esModule in place when missing on a module with default export', () => {
const moduleObj = { default: () => 'ok' };
const result = helper( moduleObj );
expect( result ).toBe( moduleObj ); // same reference, mutated in place
expect( result.__esModule ).toBe( true );
} );
it( 'does not touch modules without a default export', () => {
const moduleObj = { isShallowEqual: () => 'ok' };
const result = helper( moduleObj );
expect( result ).toBe( moduleObj );
expect( '__esModule' in result ).toBe( false );
} );
it( 'returns non-object values unchanged', () => {
expect( helper( null ) ).toBe( null );
expect( helper( undefined ) ).toBe( undefined );
const fn = function () {};
expect( helper( fn ) ).toBe( fn );
} );
it( 'sets __esModule non-enumerable so it survives a subsequent spread', () => {
const moduleObj = { default: () => 'ok' };
helper( moduleObj );
expect( Object.keys( moduleObj ) ).not.toContain( '__esModule' );
expect(
Object.getOwnPropertyDescriptor( moduleObj, '__esModule' )
.enumerable
).toBe( false );
} );
} ); |
|
Gutenberg will be releasing a patch to resolve this. WordPress/gutenberg#78917 |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Brings the
Core e2e tests - Gutenberg nightlyjob back to green. Four tests were failing on the 10.8.0 release run (job); they decompose into three independent issues.1. Cart block crashes under Gutenberg trunk (3 failures)
Affects
tests/e2e-pw/tests/product/product-create-simple.spec.ts(virtual / non-virtual / downloadable).Gutenberg's new esbuild-based bundling pipeline appends
wp.X = Object.assign({}, wp.X)to everywindow.wp.*global to materialize getter-based exports.Object.assignonly copies enumerable own properties, so the non-enumerable__esModule: trueflag is dropped. That breaks webpack's default-import interop (__webpack_require__.nfalls back to returning the whole namespace object instead of.default), and any external consumer doingimport isShallowEqual from '@wordpress/is-shallow-equal'ends up calling the namespace object as a function. The first observable failure was the Cart block's error boundary: inclient/blocks/assets/js/data/cart/push-changes.ts:197,isShallowEqual(localState.customerData, …)throws and crashes the whole block.This is an upstream Gutenberg regression; filed as WordPress/gutenberg issue 78697 so the build pipeline can preserve
__esModule. Until that lands, this PR routesisShallowEqualthrough a small namespace-aware shim that picks.defaultif present, falls back to the new namedisShallowEqualexport, and finally to the module object itself. Works on both WordPress core's bundled Gutenberg and Gutenberg trunk.12 default imports in
client/blocks/assets/js/get redirected throughassets/js/utils/is-shallow-equal.ts; the one default import inpackages/checkout/filter-registry/index.tsgets the workaround inlined because it crosses a package boundary.2. Post title locator stale on Gutenberg trunk (1 failure)
Affects
tests/e2e-pw/tests/wp-core/create-post.spec.ts.canvas.getByLabel( /Add title|Block: Title/ )no longer matched the title field, but the underlying issue was actually two:textboxwhose accessible name isAdd title, but in recent Gutenberg builds it's no longer surfaced througharia-label. Switched togetByRole( 'textbox', { name: /^(Add title|Block: Title)$/ } ), which works on both Gutenberg trunk and older releases that exposed the same accessible name viaaria-label/Block: Title.getCanvasin thee2e-utils-playwrightpackage checks foriframe[name="editor-canvas"]synchronously. On recent Gutenberg builds the iframe mounts slightly after initial paint, sogetCanvaswas returning the outer page and the locator then waited forever against the wrong frame. Added a short, soft-failing wait on the iframe so older Gutenberg releases (no iframe) keep working.Screenshots or screen recordings:
Not applicable — e2e + a small runtime shim.
How to test the changes in this Pull Request:
can create a simple virtual productcan create a simple non virtual productcan create a simple downloadable productcan create new postgutenberg-stable(older Gutenberg) to confirm no regression on the previously-working path.Testing that has already taken place:
bph/gutenberg) before the fix: console showedTypeError: vr(...) is not a functionoriginating inwc-blocks-data.jsat theisShallowEqual(localState.customerData, …)call site, and the cart hit its error boundary.6 passed (38.6s), including the[site setup]setup project).pnpm --filter=block-library ts:checkreports the same number of errors before and after this PR (2096 pre-existing errors, none introduced by the change).Milestone
Changelog entry
Changelog entries already added:
plugins/woocommerce/changelog/e2e-fix-gutenberg-nightly-title-locatorplugins/woocommerce/changelog/fix-gutenberg-nightly-is-shallow-equal-default-import