-
Notifications
You must be signed in to change notification settings - Fork 217
Node 20 upgrade lint fixes #4606
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Commented on a couple of instances where we seem to exceed the intended scope of this PR but they are not necessarily merge blockers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
✅ Linter runs with no errors on node 16.18.0 and 20.18.1
✅ npm run build:webpack
runs on node 16 with no errors ( It fails on 20 but that's not a focus of this PR)
✅ Smoke tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can only test with node 16. node 20 fails setup for me.
✅ npm run lint:js
✅ npm run build:webpack
✅ Did not find any functional changes on visual code inspection. I did notice we removed some dependencies from a couple of useSelect
s, but it looks like they were stable fields anyways.
Source PR #4264
Changes proposed in this Pull Request:
As part of the Node 20 upgrade project, this PR extracts most of the required lint fixes from the source PR (#4264). The changes here are compatible with both versions (16 and 20).
Basically, spacing of JS docblock parameters and moving function definitions before their usage.
Testing instructions
add/node-20-upgrade-lint-fixes
)npm run build:webpack
Changelog entry
Changelog Entry Comment
Comment
Post merge