Skip to content

Add archiver patches back to fix following symlinks on export#3060

Merged
wojtekn merged 5 commits intotrunkfrom
add-archiver-patches-back
Apr 13, 2026
Merged

Add archiver patches back to fix following symlinks on export#3060
wojtekn merged 5 commits intotrunkfrom
add-archiver-patches-back

Conversation

@wojtekn
Copy link
Copy Markdown
Contributor

@wojtekn wojtekn commented Apr 10, 2026

Related issues

How AI was used in this PR

I used AI to regenerate patches and revert unnecessary changes from the previous PR.

Proposed Changes

I propose adding patches that add follow-symlinks option to archiver.

Testing Instructions

  1. Create a Studio site
  2. Symlink directory e.g. plugin
  3. Cnofirm plugin loads on site
  4. Export site
  5. Confirm directory is included in package, and it's not a symlink

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@wojtekn wojtekn self-assigned this Apr 10, 2026
@wojtekn wojtekn requested a review from fredrikekelund April 10, 2026 14:02
@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Apr 10, 2026

📊 Performance Test Results

Comparing 1428970 vs trunk

app-size

Metric trunk 1428970 Diff Change
App Size (Mac) 1286.16 MB 1286.16 MB 0.00 MB ⚪ 0.0%

site-editor

Metric trunk 1428970 Diff Change
load 1611 ms 1824 ms +213 ms 🔴 13.2%

site-startup

Metric trunk 1428970 Diff Change
siteCreation 8116 ms 9120 ms +1004 ms 🔴 12.4%
siteStartup 4928 ms 4315 ms 613 ms 🟢 -12.4%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

@wojtekn wojtekn requested a review from a team April 10, 2026 15:24
The @types/archiver patches added followSymlinks to CoreOptions, but
this is unnecessary — ARCHIVER_OPTIONS in constants.ts uses plain object
inference so followSymlinks is never checked against CoreOptions. The CLI
patch also broke CI because install:bundle uses --omit=dev, meaning
@types/archiver is absent when patch-package runs.
Copy link
Copy Markdown
Contributor

@epeicher epeicher left a comment

Choose a reason for hiding this comment

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

Thanks @wojtekn for adding those! I have tested running npm install and running npm start and I don't see any errors. I have tested exporting a site with symlinks and I see the symlinked plugin folder in the new site. I have added a commit that removes the types @types+archiver+7.0.0.patch because that patch was not necessary and it was also breaking the CI. Once the CI passes this could be merged. LGTM!

Symlink site Imported previously exported site
Image Image

The @types/archiver patches added followSymlinks to CoreOptions, but
@types/archiver@7 dropped followSymlinks from CoreOptions (it's still
supported at runtime). The CLI patch also broke CI because install:bundle
uses --omit=dev, meaning @types/archiver is absent when patch-package
runs. Restore the `as archiver.ArchiverOptions` casts at call sites.
@fredrikekelund
Copy link
Copy Markdown
Contributor

Let's remove the as archiver.ArchiverOptions type assertions from #3050. That can easily hide errors in the future

@fredrikekelund
Copy link
Copy Markdown
Contributor

Ah, I see now that we'd also need to patch @types/archiver for that to work… I think we should either do that or add a line like this:

// @ts-expect-error The `followSymlinks` option comes from a patch

The latter option is probably easier and clearer, actually.

@epeicher
Copy link
Copy Markdown
Contributor

Let's remove the as archiver.ArchiverOptions type assertions from #3050. That can easily hide errors in the future

@fredrikekelund adding @types/archiver breaks the build as they are added to the devDependencies and it's dropped when patch-package runs (some details here)

I'm curious, why do you think it can hide errors in the future?

add a line like this:

// @ts-expect-error The `followSymlinks` option comes from a patch
The latter option is probably easier and clearer, actually.

That seems a good approach.

@fredrikekelund
Copy link
Copy Markdown
Contributor

I'm curious, why do you think it can hide errors in the future?

Because it effectively disables type checking. Typescript won't complain about misnamed, missing, or unrecognized props.

I think the @ts-expect-error makes it clear that we're doing something custom, which is great.

@wojtekn
Copy link
Copy Markdown
Contributor Author

wojtekn commented Apr 13, 2026

@epeicher @fredrikekelund I remove those casts and added explicit @ts-expect-error where it was needed.

@epeicher
Copy link
Copy Markdown
Contributor

Because it effectively disables type checking. Typescript won't complain about misnamed, missing, or unrecognized props.

That makes sense, thanks for the explanation @fredrikekelund. That's a strong reason to always try to avoid type casting 👍

@wojtekn wojtekn merged commit 05d985f into trunk Apr 13, 2026
10 checks passed
@wojtekn wojtekn deleted the add-archiver-patches-back branch April 13, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants