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

deps: Bump nan for compatibility with newer NodeJS #97

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 17, 2023

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

  • After this change, ppm can now be built locally on my machine with the node on my machine being the latest current NodeJS 20.6.1.
  • This shouldn't hurt compatibility with older NodeJS versions, so CI should pass!

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.
@savetheclocktower
Copy link
Contributor

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
Copy link
Member Author

DeeDeeG commented Sep 17, 2023

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
Copy link
Contributor

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
Copy link
Member Author

DeeDeeG commented Sep 22, 2023

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.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are obviously quite small. It's not even a major version change, so doesn't seem to concerning.

CI Is passing without issue (unlike elsewhere lol), and your description of the changes looks rad.

Sorry this one took time to review, but things look good here, and lets get this merged!

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 29, 2023

Thanks for the review, merging!

Not messing with the CI test matrix for now.

@DeeDeeG DeeDeeG merged commit 7b4a709 into master Sep 29, 2023
@DeeDeeG DeeDeeG deleted the deps-update-nan-for-compatibliity-with-newer-Node branch December 13, 2023 06:18
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.

3 participants