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

fix: pnpm collection of optional dependencies #8957

Merged

Conversation

indutny-signal
Copy link
Contributor

node-module-collector should not expect optional dependencies to be present on disk. If require('dep/package.json') throws for optional dependency - it is completley safe to continue as if it was never installed.

Copy link

changeset-bot bot commented Mar 13, 2025

🦋 Changeset detected

Latest commit: 96f263e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

node-module-collector should not expect optional dependencies to be
present on disk. If `require('dep/package.json')` throws for optional
dependency - it is completley safe to continue as if it was never
installed.
@indutny-signal indutny-signal force-pushed the fix/pnpm-optional-deps branch from d07cfec to d63dda5 Compare March 13, 2025 23:47
@indutny-signal
Copy link
Contributor Author

Sorry, it was supposed to be a patch release. I messed up the changeset at first

@mmaietta
Copy link
Collaborator

Can you elaborate on this? I'm not sure I follow why it's safe to continue

If require('dep/package.json') throws for optional dependency - it is completley safe to continue as if it was never installed.

@indutny-signal
Copy link
Contributor Author

Can you elaborate on this? I'm not sure I follow why it's safe to continue

The way pnpm module collector is written right now is that it will iterate through all dependencies both optional and not, and will require('package.json') of every single one of them. In our case we have fs-xattr in optional dependencies and it cannot be installed on Windows, so the build on Windows was failing with an error from pnpm collector saying that fs-xattr cannot be "require"d.

@mmaietta
Copy link
Collaborator

Oh crap, that totally makes sense! Thanks for flagging this!

I wonder if we'll have the same issue with yarn/npm then with optional dependencies? I'll need to take a deeper look at the code now that my laptop has been repaired

@mmaietta mmaietta merged commit ad151b9 into electron-userland:master Mar 16, 2025
15 checks passed
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.

2 participants