Fix @php-wasm/web icu.dat dynamic import path in dist bundle#3708
Merged
Conversation
Walks every installed @php-wasm/* and @wp-playground/* package under
node_modules, extracts dynamic import("./...dat|so|wasm") specs from
index.js and index.cjs, and asserts each resolves to an existing file
relative to the package directory. Catches the class of regression where
a Vite/Rollup transform emits an out-of-tree path (e.g. icu.dat ending
up under @php-wasm/intl/ instead of @php-wasm/web/shared/).
Mirrored across the ESM (node:test) and CommonJS (Jest) test projects.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes the icu.dat dynamic-import path in the @php-wasm/web published bundle (which broke after PR #3566 moved the import one directory up) and adds regression tests that verify dynamic asset imports in built npm packages resolve to real files.
Changes:
- Add
assets.spec.tsregression tests for ESM (node:test) and CommonJS (Jest) that scan built bundles forimport("…dat|so|wasm")and assert the paths exist. - Wire the new ESM spec into
run-tests.ts. - (Forthcoming commit) Rewrite the
viteExternalDynamicImportstransform inpackages/php-wasm/web/vite.config.tsto emit./shared/<file>regardless of source depth.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/playground/test-built-npm-packages/es-modules-and-vitest/tests/assets.spec.ts | New ESM regression test scanning installed packages' bundles for dynamic asset imports. |
| packages/playground/test-built-npm-packages/es-modules-and-vitest/run-tests.ts | Registers the new assets.spec.ts with the ESM test runner. |
| packages/playground/test-built-npm-packages/commonjs-and-jest/tests/assets.spec.ts | CommonJS/Jest equivalent of the ESM regression test. |
Comments suppressed due to low confidence (1)
packages/playground/test-built-npm-packages/commonjs-and-jest/tests/assets.spec.ts:1
- This
toEqualpattern compares three identical fields just to surface diagnostic context on failure. A simpler and clearer alternative isexpect(fs.existsSync(resolved)).toBe(true)paired with a message via Jest's third arg pattern or a custom message string — or use the ESM file'sassert.ok(..., message)approach for parity between the two specs. The current form is unnecessarily verbose and obscures the actual assertion.
const path = require('path');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,61 @@ | |||
| const path = require('path'); | |||
Comment on lines
+35
to
+37
| describe('Built package dynamic asset imports', () => { | ||
| getInstalledPackages().forEach((pkg) => { | ||
| it(`${pkg.name} dynamic asset imports resolve to files in dist`, () => { |
Comment on lines
+31
to
+33
| describe('Built package dynamic asset imports', () => { | ||
| getInstalledPackages().forEach((pkg) => { | ||
| it(`${pkg.name} dynamic asset imports resolve to files in dist`, () => { |
Comment on lines
+12
to
+17
| const importRegex = new RegExp( | ||
| `import\\(\\s*["']((?:\\./|\\.\\./)[^"']+\\.(?:${assetExtensions.join( | ||
| '|' | ||
| )}))["']\\s*\\)`, | ||
| 'g' | ||
| ); |
After PR #3566 moved the icu.dat dynamic import from src/lib/extensions/ intl/with-intl.ts (depth 3) to src/lib/extensions/load-extensions.ts (depth 2), the viteExternalDynamicImports transform's hard-coded '../../../' prefix produced 'import("../intl/shared/icu.dat")' in the dist bundle — a path that resolves outside the package to a non-existent @php-wasm/intl/shared/icu.dat. Downstream Vite consumers failed with "Failed to resolve import" against that path. Derive the output as './shared/<file>' from the specifier's tail so the transform stays correct regardless of where the import statement lives under src/. The companion regression test in test-built-npm-packages/{es-modules-and-vitest,commonjs-and-jest}/tests/ assets.spec.ts now passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
adamziel
approved these changes
May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
viteExternalDynamicImportstransform inpackages/php-wasm/web/vite.config.tsso the bundledindex.js/index.cjsresolveicu.datfrom./shared/icu.datnext to the bundle.test-built-npm-packagesregression spec that walks every installed@php-wasm/*and@wp-playground/*package, extracts dynamicimport("./...dat|so|wasm")specs from eachindex.js/index.cjs, and asserts they resolve to real files relative to the package directory.Fixes the downstream Vite consumer error:
Why
PR #3566 ("Support custom PHP.wasm extensions") moved the dynamic
import("icu.dat")fromsrc/lib/extensions/intl/with-intl.ts(3 directories belowsrc/) tosrc/lib/extensions/load-extensions.ts(2 directories below). The transform was hard-coded for the deeper location, so after the move Rollup's relative-path normalization leaves a leading../intl/segment in the output. The published bundle ends up withimport("../intl/shared/icu.dat"), which resolves tonode_modules/@php-wasm/intl/shared/icu.dat— a path that does not exist.The regression test exists so any future relocation of a dynamic-asset import in the WASM packages can't ship to npm unnoticed.
Changes by commit
[Tests] Verify dynamic asset imports resolve in built npm packages— addsassets.spec.tsto both the ESM (node:test) and CommonJS (Jest) projects underpackages/playground/test-built-npm-packages/. Wires the ESM spec intorun-tests.ts; Jest auto-discovers viatestMatch. On its own this commit fails CI against current trunk, which is the intended demonstration.Fix dynamic icu.dat import path in @php-wasm/web bundle(to follow) — rewrites theviteExternalDynamicImportstransform to derive a./shared/<file>output regardless of source-file depth undersrc/. Turns the new regression test green.Test plan
@php-wasm/webbundle —import("../intl/shared/icu.dat") -> .../@php-wasm/intl/shared/icu.dat does not exist.@php-wasm/webbundle after the transform fix.test-built-npm-packagesES Modules and CommonJS jobs fail on the tests-only commit; turn green after the vite-fix commit lands.@php-wasm/webfrom this branch's HEAD and confirm it no longer errors.