Skip to content

fix: Correctly zip local (and aliased) packages #1685

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darthmaim
Copy link

@darthmaim darthmaim commented May 27, 2025

Overview

I've fixed handling of local (and aliased) packages when building the zip.

Before this fix it was not possible to include local dependencies in the sources zip, which for example can often happen in monorepos.

Testing

I've added a test case that tests the new behavior, and I've also tested this with a combination of scoped and unscoped packages.

Notes

  • The should download local packages and produce a valid build when zipping sources test case is basically just a copy/paste of the existing should download packages and produce a valid build when zipping sources test, the only difference being the local dependencies. It might be nicer to just include the local dependencies in the original test case?
  • I've created the local test module in e2e/tests/__fixtures__/local-package. I originally wanted to create it with project.addFile('local/package.json'), but pnpm.listDependencies could not parse the output with the nested dependencies. This could be fixed by removing the -r option, but it was explicitly added in fix(zip): List all private packages correctly in a PNPM workspace #520, so I opted to store the local package outside of the extension directory.
  • downloadDependency needs to be able to resolve relative dependencies, so I've added a new cwd option similar to the one in listDependencies
  • As my testcase uses the same dependency twice to test both file: (usual default for local dependencies) and link: (what pnpm resolves workspace: to), pnpm creates an alias for one of them. downloadPrivatePackages did not create valid overrides for aliased packages, so I had to fix this too.
  • It might be nicer to use for example npm-package-arg for parsing the package ids than using regex. Edit: npm-package-arg doesn't support parsing aliases.

Copy link

netlify bot commented May 27, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 4e52907
🔍 Latest deploy log https://app.netlify.com/projects/creative-fairy-df92c4/deploys/6835b59172e6a100081fabf6
😎 Deploy Preview https://deploy-preview-1685--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@darthmaim darthmaim force-pushed the fix/zip-linked-packages branch from 8038445 to 961f783 Compare May 27, 2025 12:51
@darthmaim darthmaim force-pushed the fix/zip-linked-packages branch from 961f783 to 4e52907 Compare May 27, 2025 12:52
@darthmaim
Copy link
Author

darthmaim commented May 29, 2025

I did some more tests today, and while this does fix the handling of link: and file: dependencies, it does not yet fully fix my initial issue with "downloading" workspace:* dependencies.

The generated package.json will look like this:

  "dependencies": {
    "@local/dependency": "workspace:*",
  },
  "resolutions": {
    "@local/dependency@link:../../packages/dependency": "file://./.wxt/local_modules/local-dependency-0.0.1.tgz"
  }

Trying to install this with npm will result in Unsupported URL Type "workspace:".

I'll look into adding special handling for workspace: dependencies in the next few days, maybe in a follow up PR.

Maybe downloaded workspace: dependencies should just be directly rewritten directly without using resolutions:

  "dependencies": {
    "@local/dependency": "file://./.wxt/local_modules/local-dependency-0.0.1.tgz",
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant