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.
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
Add support for Node.js >=14.20.1 <19 #4071
Add support for Node.js >=14.20.1 <19 #4071
Changes from all commits
0ddddce
2dc3185
9fe5086
c2be7fa
a77443f
11de409
6ddf265
77f11e0
ba8f84b
ae5c780
9f4cf53
c82b6ab
35bab54
060a2c8
b77ff74
a13e9c9
5926e4f
c54e795
d98158b
b0bbeed
aaff6a7
59040e8
ef195cc
9be7e50
0a396cb
a907736
a9e2db0
79d6f15
005e3e6
5319181
ce5f732
dd3ce63
f9c56ce
2e073e4
5070935
d2d4714
fa897a8
90e7b2a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
as noted. Build repo uses this file to build OpenSearch Dashboards so we should take this into consideration if the build system needs to publish CI Runner images with Node 18 installed.
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.
@ananzh are we going to create a subsequent PR for this or fix this one?
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.
main
will continue to have 18;2.8
will use 16.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Why the non-null assertion instead of optional chaining?
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 fix is due to this error
The problem is with the
childProcess.pid
value. In Node.js, the pid is undefined if the child process has not been successfully spawned (or if the child process has already exited). Hence,childProcess.pid
is of typenumber | undefined
. ThetreeKillAsync
function expects a PID of type number, notnumber | undefined
. Therefore, we see ts throws an error.!
allows ts to treatchildProcess.pid
as type number rather thannumber | undefined
. An optional chainchildProcess.pid?
would not solve the issue because it would still result in undefined if pid doesn't exist, and treeKillAsync requires a number. Here is the result of using?
: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.
Right - I was suggesting that what you really needed here was a optional chaining with a fallback value so that you never pass
undefined
totreeKillAsync
. The non-null assertion doesn't do anything other than tell typescript not to worry about the potential ofundefined
- but ifpid
is undefined, you'll still pass that to a function that isn't prepared to handle that. What you probably want instead is a type guard, so that you only calltreeKillAsync
whenpid
is defined.