Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vite build: Mark all imported modules as external to avoid bundling them with released packages. #1586

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jul 8, 2024

Without this PR, many of the released packages bundle other packages – both from this repo and external ones.

For example, @wp-playground/wordpress bundles @php-wasm/node which caused multiple copies of that package to be loaded and create a variable identity issues such as #1579.

This PR marks all the dependent packages as external and ensures the published packages are lean and only contains their own code.

Testing instructions

Run nx build php-wasm-node and confirm the resultingdist/packages/playground/client/index.js file and other built npm-published modules do not bundle any external modules. The only exception should be the cli packages.

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Type] Repo / Project Management labels Jul 8, 2024
@adamziel adamziel merged commit 8220ae0 into trunk Jul 9, 2024
5 checks passed
@adamziel adamziel deleted the vite-mark-all-modules-as-external branch July 9, 2024 06:50
adamziel added a commit that referenced this pull request Jul 9, 2024
## Motivation for the change, related issues

Follows up on
#1586

Extracting external packages names from the `package.json` files did not
work as expected due to `cwd` misconfiguration in CI. This PR replaces
that logic with a regexp matching the name of the packages from this
monorepo to avoid maintaining the paths entirely.

## Testing instructions

Run `nx build php-wasm-node` and confirm the
resulting`dist/packages/playground/client/index.js` file and other built
npm-published modules do not bundle any external modules. The only
exception should be the `cli` packages.
adamziel added a commit that referenced this pull request Jul 9, 2024
Follows up on
#1589 and #1586

Treating package IDs matching `node_modules` as external works, but
causes Rollup to substitute the module name with the absolute module
path on the host machine, e.g. `import "ini"` becomes `import
"/home/runner/work/wordpress-playground/wordpress-playground/node_modules/ini/lib/ini.js"`
which is not what we want.

This PR sources the external packages names from the package.json file
to both treat all these imports as external AND preserve their original
import identifier.

 ## Testing instructions

Run nx build php-wasm-node and confirm the resultingdist/packages/php-wasm/index.js file and other built npm-published modules do
import external modules using their name, not their absolute path.
adamziel added a commit that referenced this pull request Jul 9, 2024
Follows up on
#1589 and #1586

Treating package IDs matching `node_modules` as external works, but
causes Rollup to substitute the module name with the absolute module
path on the host machine, e.g. `import "ini"` becomes `import
"/home/runner/work/wordpress-playground/wordpress-playground/node_modules/ini/lib/ini.js"`
which is not what we want.

This PR sources the external packages names from the package.json file
to both treat all these imports as external AND preserve their original
import identifier. It also adds the required `package.json` building
steps to the `@php-wasm/universal` package and makes the
`@php-wasm/stream-compression` package public to ensure the published
packages will reference the correct dependencies.

 ## Testing instructions

Run nx build php-wasm-node and confirm the
resultingdist/packages/php-wasm/index.js file and other built
npm-published modules do import external modules using their name, not
their absolute path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Repo / Project Management
1 participant