Skip to content

Remove postinstall script in apps/cli by using new archiver strategy - #3784

Merged
fredrikekelund merged 13 commits into
trunkfrom
f26d/remove-archiver-patch
Jun 16, 2026
Merged

Remove postinstall script in apps/cli by using new archiver strategy#3784
fredrikekelund merged 13 commits into
trunkfrom
f26d/remove-archiver-patch

Conversation

@fredrikekelund

@fredrikekelund fredrikekelund commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Related issues

How AI was used in this PR

Claude was used to iteratively reach the desired state. I bounced around ideas for achieving the goals, iterated on ways to simplify the implementation, and relied on it heavily to simplify tests.

Proposed Changes

npm v12 will stop running postinstall scripts by default. The wp-studio package currently runs a postinstall script to patch the archiver npm module using patch-package.

This PR removes the need for that patch by eliminating the use of archiver.directory() and instead doing the globbing ourselves and calling archiver.file() for each file in our list. The PR also removes the patch-package dependency and the actual patch.

Moreover, I took the opportunity to update archiver to v8, which is the latest version.

Testing Instructions

  1. Ensure that creating preview sites works as expected
  2. Ensure that exporting a site works as expected

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@fredrikekelund
Remove archiver patch
0a91599 
Call `glob` with the right options and then pass every file path to `Archiver.file`. Also, update archiver to v8.
@fredrikekelund
Simplify tests and rely more on memfs
733817b
@fredrikekelund
fredrikekelund requested a review from a team June 11, 2026 14:10
@fredrikekelund fredrikekelund self-assigned this Jun 11, 2026
@wpmobilebot

wpmobilebot commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

📊 Performance Test Results

Comparing c546dc6 vs trunk

app-size

Metric trunk c546dc6 Diff Change
App Size (Mac) 2334.13 MB 2328.95 MB 5.18 MB 🟢 -0.2%

site-editor

Metric trunk c546dc6 Diff Change
load 1739 ms 1776 ms +37 ms ⚪ 0.0%

site-startup

Metric trunk c546dc6 Diff Change
siteCreation 8545 ms 8503 ms 42 ms ⚪ 0.0%
siteStartup 3901 ms 3889 ms 12 ms ⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)
bcotrim
bcotrim approved these changes Jun 11, 2026
View reviewed changes

@bcotrim bcotrim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Note:

  • An exported site is missing empty folders, unlike trunk.
    I don't think it will have any impact, but wanted to flag to raise the flag.
  • I fixed unit tests and merge conflicts, feel free to adjust as you see necessary
@bcotrim
bcotrim requested a review from a team as a code owner June 12, 2026 07:24
epeicher
epeicher reviewed Jun 12, 2026
View reviewed changes

@epeicher epeicher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, @fredrikekelund. Removing the postinstall will be a great step! I have tested it, and I have found a couple of issues that I think should be fixed:

  1. The preview site does not include the symlink to a single file
  2. When exporting the site and importing it as a new site, I am getting the error in the screenshot below
Original site Preview site Site created from the Exported site
Image Image Image

Please let me know if you have any questions. In a nutshell, the symlink to folders seems to be working fine, but the symlink to single files does not seem to be working for me.
@fredrikekelund
Directly append file data to archive
b197bee 
When a file symlink is passed to `Archive.file`, archiver will append an actual symlink to the archive instead of the targeted file. `Archiver.append` just appends the raw data, which is why I opted for it.
@fredrikekelund

fredrikekelund commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Kudos for the thorough testing, @epeicher 🙏 I pushed a fix for the file symlink issue. I'll run some more testing tomorrow morning before landing this. It looks like the "Site created from the Exported site" issue you found also came from the file symlink thing
@fredrikekelund

fredrikekelund commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed that file symlinks are included properly in both types of exports with this new approach ✅
@fredrikekelund

fredrikekelund commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

AI review raised a real potential issue: fs.createReadStream opens a file descriptor that isn't closed until the archive is finalized. For a large site directory, this means that we could get EMFILE errors if the system thinks we're holding too many file descriptors. To solve for this, I changed the strategy from using Archiver.append() to using Archiver.file() again, but calling realpath on the source path first.

I've confirmed that the new approach works as expected, too ✅
fredrikekelund
fredrikekelund commented Jun 16, 2026
View reviewed changes
Comment thread apps/cli/__mocks__/fs.cjs

@fredrikekelund fredrikekelund Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change wasn't crucial to the core purpose of this PR, but it's how Vitest recommends mocking the fs module. We don't need to reimplement what memfs exports. The point of that module is to provide the same API as fs, just backed by a temporary file system.
fredrikekelund
fredrikekelund commented Jun 16, 2026
View reviewed changes
Comment thread apps/cli/lib/tests/validation.test.ts

@fredrikekelund fredrikekelund Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are unrelated to the core purpose of this PR – they're just simplifications I was able to make from tweaking the fs mock.
fredrikekelund
fredrikekelund commented Jun 16, 2026
View reviewed changes
Comment thread apps/cli/package.json
Comment on lines -34 to +41
"@php-wasm/node": "3.1.36",
"@php-wasm/universal": "3.1.36",
"@php-wasm/util": "3.1.36",
"@php-wasm/node": "3.1.38",
"@php-wasm/universal": "3.1.38",
"@php-wasm/util": "3.1.38",
"@vscode/sudo-prompt": "^9.3.2",
"@wordpress/i18n": "^6.20.0",
"@wp-playground/blueprints": "3.1.36",
"@wp-playground/cli": "3.1.36",
"@wp-playground/common": "3.1.36",
"@wp-playground/storage": "3.1.36",
"@wp-playground/wordpress": "3.1.36",
"archiver": "^7.0.1",
"@wp-playground/blueprints": "3.1.38",
"@wp-playground/cli": "3.1.38",
"@wp-playground/common": "3.1.38",
"@wp-playground/storage": "3.1.38",
"@wp-playground/wordpress": "3.1.38",

@fredrikekelund fredrikekelund Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These version changes are unrelated to the core purpose of this PR. #3829 mistakenly downgraded the Playground dependencies in apps/cli to 3.1.36, so I took the opportunity to reset them back to 3.1.38.

This is also what caused the change in installer size. Previously, we had different Playground dependency trees: one for 3.1.36 and another for 3.1.38
@fredrikekelund
fredrikekelund enabled auto-merge (squash) June 16, 2026 09:18
@fredrikekelund
fredrikekelund merged commit 5f43669 into trunk Jun 16, 2026
13 checks passed
@fredrikekelund
fredrikekelund deleted the f26d/remove-archiver-patch branch June 16, 2026 09:39
@fredrikekelund fredrikekelund mentioned this pull request Jun 24, 2026
Merged
1 task
fredrikekelund added a commit that referenced this pull request Jun 24, 2026
@fredrikekelund
Fix bloated installer size (#3943)
f7f8811 
Primarily trying to reconcile the big jump in installer size that we saw
recently

## Related issues

<!--
Link a related issue to this PR. If the PR does not immediately resolve
the issue,
for example, it requires a separate deployment to production, avoid
using the "Fixes" keyword and use "Related to" instead.
-->

N/A

## How AI was used in this PR

<!--
Help reviewers understand what to look for and verify that you've
reviewed the code yourself.
-->

N/A

## Proposed Changes

<!--
Explain the intent of this PR:
- What problem does it solve, or what need does it address?
- How does it affect the user (new capability, fix, performance,
accessibility, etc.)?
- Note any user-visible behavior changes or trade-offs.

Focus on the "why" and the user impact. Avoid listing modified files or
describing implementation mechanics — the diff already shows that.
-->

#3829 bumped the installer size
significantly. We discussed this just after that PR was merged, and I
made an effort in #3784 to fix
the problem. Turns out that didn't work, because the installer size is
still huge.

This PR fixes the problem by restoring the `--omit=dev` option in the
`install:bundle` script that's used to package `apps/studio`
dependencies when building the app.

## Testing Instructions

<!--
Add as many details as possible to help others reproduce the issue and
test the fix.
"Before / After" screenshots can also be very helpful when the change is
visual.
-->

See installer size results from performance metrics CI job

## Pre-merge Checklist

<!--
Complete applicable items on this checklist **before** merging into
trunk. Inapplicable items can be left unchecked.

Both the PR author and reviewer are responsible for ensuring the
checklist is completed.
-->

- [ ] Have you checked for TypeScript, React or other console errors?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@fredrikekelund @wpmobilebot @epeicher @bcotrim