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

Remove workaround for node-gyp issues #7722

Closed
MRayermannMSFT opened this issue Feb 1, 2024 · 1 comment
Closed

Remove workaround for node-gyp issues #7722

MRayermannMSFT opened this issue Feb 1, 2024 · 1 comment
Assignees
Labels
🧪 engineering Related to some internal engineering improvements ✅ merged A fix for this issue has been merged
Milestone

Comments

@MRayermannMSFT
Copy link
Member

It looks like we can finally remove our workarounds for node-gyp nodejs/node-gyp#2584.

The fix shipped in node-gyp 9.4.0: https://github.com/nodejs/node-gyp/releases/tag/v9.4.0

Electron 28 comes with node 18.18.2, which comes with npm 9.8.1, and that has node-gyp 9.4.0!
See: https://github.com/npm/cli/blob/facda25daab7cdd5f6aaad2260379aedeef33caa/package.json#L95

Electron 29 comes with node 20.9.0, which comes with npm 10.1.0, and that has node-gyp 9.4.0!
See: https://github.com/npm/cli/blob/39e2b87f283a80af8c79eb76096c017127b2d63f/package.json#L96

So I think we should be good to go. Perhaps as a smoke test, create a branch which installs like 4 copies each of our native modules (via the use of npm aliases), and run a couple of local and CI WIndows builds, making sure npm install/npm run init never fails.

This should probably be done after updating to Electron 29, after we have handled any breaking changes associated with moving to node 20.

@MRayermannMSFT MRayermannMSFT added the 🧪 engineering Related to some internal engineering improvements label Feb 1, 2024
@MRayermannMSFT MRayermannMSFT added this to the 1.34.0 milestone Feb 1, 2024
@JasonYeMSFT
Copy link
Contributor

This has been done for Electron 28. I'll close this issue and maybe re-open it if Electron 29 regresses and a new workaround needs to be implemented.

@JasonYeMSFT JasonYeMSFT added the ✅ merged A fix for this issue has been merged label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧪 engineering Related to some internal engineering improvements ✅ merged A fix for this issue has been merged
Projects
None yet
Development

No branches or pull requests

2 participants