-
Notifications
You must be signed in to change notification settings - Fork 540
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
feat: build binaries with node@16 #2508
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f1aa86b
to
7ef6c94
Compare
e7f5fce
to
618a721
Compare
maxjeffos
approved these changes
Jan 4, 2022
This was referenced Jan 21, 2022
Closed
npm@8's only breaking change is a lack of node@10 support.
It worked before because tests were using "npx ts-node" which adds a handler too.
Because of how various tooling may hook into error events, enforcing a limit is too naive and may cause further unexpected failure scenarios. Until such a scenario causes us problems, it's safer not to handle it.
186bc96
to
c46ac98
Compare
Need to also default to node@16 and npm@8 in dev/ci as dependencies may contain native modules (.node) which need to be installed using the same node version for compatibilty in binaries.
c46ac98
to
d2fd763
Compare
This was referenced Aug 30, 2022
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original goal of this PR was to only migrate our CI and dev environment to node@16. However, I've squashed #2507 into this as our binaries need to use the same Node version as the build environment. So the main visible change in this PR is now the fact that we ship our binaries with node@16.
To Do
Future PR
These steps can be done later as #2599 will change how we current build binaries.
Node 16 Binaries
Node 16 is the latest LTS. We already have a Node 16 test pipeline and binaries are tested via production/smoke tests.
Node 16 in Dev and CI
We are currently using Node 14 in development. Let's upgrade to Node 16 LTS before we get left behind.
One major benefit is that darwin/arm64 is only support from Node 16 and above.
Node 14 will still be supported in development to avoid suddenly breaking contributors' environments. We should give people some time to migrate.
Node 16 ships with npm@8. npm@8 is not supported on Node 10. It doesn't fail hard, but does print warnings about it.
So we'll either need to:
This PR is going for the last approach so that we're not left behind. Our pipeline still works even with the warnings so it's a matter of removing Node 10 before it stops working.
Failing "Unexpected Error" Tests
Because
npx ts-node
now prints the deprecation warning on Node 10, I had to change the unexpected errors test to usenode -r ts-node/registry
so it doesn't pollutestderr
assertions. This picked up a minor bug in our error handling module which is fixed in its own commit. The bug wasn't picked up previously because ts-node polluted the process under test with its own error handling, making the test invalid.Considering various tools may want to hook into error handling and the complexity involved in enforcing the limit, I've removed the handler limit. We can re-introduce the limit when we encounter problems related to not having one.