pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

deps: Bump nan for compatibility with newer NodeJS #97

Closed DeeDeeG closed 11 months ago

DeeDeeG commented 11 months ago

Summary

Bump nan from 2.16.0 to 2.18.0. (Tiny, tiny, single-dependency bump in yarn.lock and package-lock.json only!)

(I recommend reviewers to check the diff to see that this is a super tiny PR.)

Context: What is nan?

nan is a slightly more stable API target for compiling C/C++ addons for NodeJS projects against, attempting to hide the complexity and moving target that is the V8 engine internals beneath a more manageable, and once again a more "stable" API target.

For more info, see: https://github.com/nodejs/nan

Why do we need to update it periodically?

We occasionally need to bump this package for compatibility with newer NodeJS versions that bundle newer versions of V8, or carry different V8 patches, etc. Newer nan versions get put out from time to time to address breakages in nan's "stable" API when used against the newer/different copies of V8 found in newer NodeJS versions. Newer Node+V8 versions come out and break stability in their C/C++ interfaces, newer nan versions come out to pave over the breakage and continue to provide a stable API target over time.

Motivation for bumping this now

Allows us to bootstrap ppm on machines running the latest NodeJS 20.x

Remains untested whether ppm can fully run on NodeJS 20, but at least we know we can compile dependencies against NodeJS 14-20. Useful for keeping our Electron upgrade path smooth for the future. Not a perfect guarantee, but a great step to take nonetheless.

See: https://github.com/nodejs/release#release-schedule

Related task for the repo: Add NodeJS 20 to the test matrix, drop NodeJS 14

We should add NodeJS 20 to the test matrix for this repo soon, and drop NodeJS 14

NodeJS 20 will be an LTS version starting in October. With this PR, we should be good to start testing against it now, if we want. It should be decently mature, given it'll be LTS soon. However, it would also make just about as much sense to wait for it to be actually LTS (and therefore have better stability guarantees as a moving target of minor and patch versions to test against).

And yeah, we should drop NodeJS 14 from the test matrix, it's been EOL for a while now.

In theory we should drop NodeJS 16, since it just went EOL on September 11th, but since I think that version is still the best-supported version of Node to use around the various repos in Pulsar org, I think it's still worth testing, even if NodeJS 16 is also technically EOL already.

See:

Verification process

savetheclocktower commented 11 months ago

We should add NodeJS 20 to the test matrix for this repo soon, and drop NodeJS 14

This may or may not be relevant, since I know this is PPM we're talking about, but the version of Electron we're using is pinned to Node v14.16.0. How much does that matter? Hopefully we'll be able to move to a newer Electron soon, but that's where we are at the moment.

DeeDeeG commented 11 months ago

the version of Electron we're using is pinned to Node v14.16.0.

Hmm. I see you're right about that, and in that case yeah, I would want to keep 14 in the test matrix. With us leaning toward Node 16 everywhere, I kind of forgot Electron 12 is still on Node 14, to be honest.

(The Electron <--> Node+Chrome version table, for reference anyone reading this: https://www.electronjs.org/docs/latest/tutorial/electron-timelines. But you were right, yeah.)

Probably want to stick to three or less major versions of Node for the test matrix. But the very specific ones don't matter much as long as we test against the newest LTS and the Electron version in the core editor's major version of Node are in there, we are pretty well covered, in my experience. (I would suggest 14 + 20 + [whatever LTS between 14 and 20 we want here, somewhat arbitrarily]).

savetheclocktower commented 11 months ago

The good news is that the outlook for upgrading Electron soon-ish is suddenly very good — now that @mauricioszabo has gotten superstring performance to be much better in his Electron 25 branch. I don't think we'll have to test on Node 14 for much longer.

DeeDeeG commented 11 months ago

I really went overboard with the long PR body. This is a trivial, patch-level indirect dependency bump of nan.

(I've never had an issue with bumping nan before, personally. This came up a lot during the official atom org and atom-community org days, I seem to recall. Either due to bumping Electron, or due to testing against newer Node versions in the CI config, or users trying to build a given repo locally with newer Node versions, etc. etc. etc. We just bumped nan and it allowed compiling against newer targets. EZPZ.)

Can I get a review, whenever folks get the chance, @pulsar-edit/backend? (EDIT: meant to tag core team, oops.) If there are any questions, I can try to answer them. I feel this is a very very very very low-risk, and tiny change, though, my personal view on this. Thank you for any reviews/comments/questions/etc.

DeeDeeG commented 11 months ago

Thanks for the review, merging!

Not messing with the CI test matrix for now.