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

Preemptively unblock Node 20 #659

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Preemptively unblock Node 20 #659

merged 1 commit into from
Mar 2, 2023

Conversation

ejizba
Copy link
Contributor

@ejizba ejizba commented Feb 18, 2023

The sooner the better!

v20 will be based off of v19, so I think it's good enough to use v19 for our CI testing for now

Related to Azure/azure-functions-nodejs-library#170

@@ -218,14 +219,18 @@ describe('WorkerInitHandler', () => {
},
});

const jsonError = semver.gte(process.versions.node, '19.0.0')
Copy link
Contributor

Choose a reason for hiding this comment

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

I support the idea of this PR to preemptively support node 20 as much as possible. But I worry, are we adding additional unnecessary burden here by using node 19 as a proxy for node 20? Are we forcing ourselves to support features/updates in node 19 that may not end up making it to node 20? This change is an example. Are we sure that node 20 will still have this different behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well...it took me all of 10 minutes to fix this unit test. Can you expand on what you mean by burden?

For the record, this PR doesn't mean we actually support anything. Users have to wait for official public preview/GA announcements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I'm just saying in general. I'm not sure what other changes in node 19 that might require us to update/modify tests that won't be necessary in node 20. If you don't think this is a significant risk, I'm cool merging this and won't block on it

@ejizba ejizba merged commit 4607f46 into v3.x Mar 2, 2023
@ejizba ejizba deleted the ej/20 branch March 2, 2023 20:58
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.

2 participants