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

Override nan to support node 22 (and earlier) #8097

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

sarahec
Copy link
Contributor

@sarahec sarahec commented Jan 4, 2025

Description

Overrides nan (node native interfaces) to its current 2.22.0. This adds support for node 22 for any native V8 interfaces and fixes a remaining 6.x ajv compilation issue due to the angular tools.

Pay particular attention to the npm-shrinkwrap diffs -- you can see the nan upgrade happening in exactly two places.

Fixes #8096

Scenarios Tested

All tests not requiring a local firebase project. Also, this compiles on nixos with the sandbox (which builds all from source including extensions -- it's a very strict build which also runs the tests.)

Sample Commands

n/a

@sarahec
Copy link
Contributor Author

sarahec commented Jan 4, 2025

One question that came up on the Nix side: should I just run npm upgrade nan to change the shrinkwrap and not add anything in the package.json?

In my mind, the override in package.json makes it explicit that we're using a different (newer) nan than we would otherwise get if we regenerated the shrinkwrap from scratch (but I could be wrong).

@joehan
Copy link
Contributor

joehan commented Jan 6, 2025

One question that came up on the Nix side: should I just run npm upgrade nan to change the shrinkwrap and not add anything in the package.json?

In my mind, the override in package.json makes it explicit that we're using a different (newer) nan than we would otherwise get if we regenerated the shrinkwrap from scratch (but I could be wrong).

I think I prefer just changing the shrinkwrap here, (or scoping the override to just re2) - it previously had nan:"^2.17.0"for the affected dependency, which should resolve to the largest 2.x.y version on a fresh regen.

The override also adds a bit of potential tech debt - if/when a dependency switches to [email protected], this would pin us to 2.x.x and potentially break things.

@sarahec sarahec force-pushed the push-pmxykqqnxnuy branch from 66108f9 to 1b44339 Compare January 6, 2025 19:23
@sarahec
Copy link
Contributor Author

sarahec commented Jan 6, 2025

Agreed. Removed the override keyword and kept the shrinkwrap changes.

@sarahec sarahec force-pushed the push-pmxykqqnxnuy branch from 1b44339 to c712647 Compare January 6, 2025 19:34
@sarahec
Copy link
Contributor Author

sarahec commented Jan 6, 2025

Rebased atop master

@joehan joehan merged commit 625ae73 into firebase:master Jan 6, 2025
6 of 11 checks passed
@sarahec sarahec deleted the push-pmxykqqnxnuy branch January 6, 2025 19:39
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.

Continuing build error from older ajv under Node 22. Needs updated native interfaces.
2 participants