-
Notifications
You must be signed in to change notification settings - Fork 175
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!: update node-gyp to ^10.0.0 and fix subsequent breaking changes #1128
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1128 +/- ##
==========================================
- Coverage 76.93% 76.24% -0.69%
==========================================
Files 21 21
Lines 763 762 -1
Branches 142 142
==========================================
- Hits 587 581 -6
- Misses 122 125 +3
- Partials 54 56 +2 ☔ View full report in Codecov by Sentry. |
2341906
to
99fc935
Compare
I suppose it is breaking for node v12/14. Also, this doesn't seem to work on Windows. Well, that's a bummer. This fails:
I'm not experience enough to comment on why specifically windows won't build here as of yet. Maybe someone else has a clue. Latest cricleci before this change suceeds for Windows + Node 20, circli for this change does not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolutions
won't propagate to folks who install this module, this would be a breaking change for them. This is on my list of things to look at
I see, the Totally broken on Windows still tho, no idea what fails on the Win build system tho. IMHO node v12/14 could realistically be wontfixes, but Windows has to work first anyway. |
Hey folks, I just merged
We just defined a new policy for We're looking to make a major version bump in lockstep with all our packages sometime in the future for Node.js 20 so that we can avoid ecosystem fragmentation. |
Thanks for the update @erickzhao ! I haven't gotten around to look further into why Windows doesn't build, and I honestly don't get along well with C-anything building, especially on Windows. Really hoped node-gyp would just cover that. Bummer. Good on the upgrade policy! Now Windows should just work... PS: Tarekis and me are the same person, just a different user, sorry if that's confusing. |
Thank you for your efforts! Closing with #1157 |
I took a shot at upgrading
node-gyp
to^10.0.0
, because electron-rebuild does not work with python3.12.x
as mentioned in #1116.This change is non-breaking and backwards-compatible with python
3.11.x
since[email protected]
also is.The obvious change here is updaring
node-gyp
to^10.0.0
, but there are some errors that arise from doing so.So this PR also fixes errors introduced by this upgrade:
Removes promisifying of node-gyp commands called in the worker, and instead calls the node-gyp APIs directly as they were promisified internally as notend in the
v10.0.0
breaking changes.dev only:
@types/minipass
are set to^3.3.5
because tar has a dependency version of * but the set version is required, as recommended here.dev only: Trough peer dependencies
v10.0.0
introduces dependencies tostring-width
@^5.0.1
and^5.1.2
, which are ESM modules, and since@electron/rebuild
targets CommonJS this fails with ESM Errors. Since a yarn lockfile is used, I used yarn-specific overrides (speak: resolutions) to force the usage of4.2.3
, which does not target ESM, and thus works with a CommonJS target.This probably could be done more gracefully, but this works for now, and only affects dev, so if you'd like to have this done differently, please manually seek some versions that work and update the lockfile.