-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[6.8] Upgrade from Node.js 10 to 12 #100963
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
882dbd3 to
0ecec1c
Compare
17d689b to
75026e0
Compare
This reverts commit abda1f4dd02fccba5ee81a52f9704734729099db.
This reverts commit 59369c5906cdbea66667bea0d273c90215923abd.
|
Hmm - I can't get this to pass: I see the build went through on CI, so it's probably something local but also noting in case anyone else sees the same issue. |
|
Maybe a |
jbudz
left a comment
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.
I switched to a fresh clone and the errors went away - must have been something in my workspace that a clean wasn't catching.
Ran a build, and went through most of the applications. Didn't see anything out of place - LGTM
|
CC'ing the relevant teams for visibility, in case they would like to review these changes themselves (I did not include those teams whose tests or types were the only things that changed): @elastic/kibana-core @elastic/stack-monitoring @elastic/kibana-reporting-services @elastic/kibana-security @andrewvc |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
|
OOD |
Depends on
Known breaking changes
It's not unthinkable that there are other breaking changes that we don't know about.
See all differences between Node.js 10 and 12 here: https://nodejs.org/tr/blog/uncategorized/10-lts-to-12-lts/
Notes to reviewers
6.8at this time. However, we need it to be reviewed so it's ready in case we need it in the future if we decide to offer a custom build of Kibana 6.8 that runs on Node.js 12. At that point in time we might decide to switch the destination branch to a custom6.8-nodejs12branch.6.8branch doesn't have as good test-coverage as7.x, please be extra vigorous and do some manual testing, as relying on CI being green could give a false sense of security 🙏typescriptdependency to 3.1.x as the previous version (3.0.3) didn't work with the new version of Node.js and/or the new version of@types/node.--tls-min-v1.0flag tobin/kibana.I think that's the only place I need to add it.Update: Now also added tobin/kibana.bat(thanks to @mieciu for spotting this)@types/nodealso meant that a few changes needed to be made to our source code to please the types.@testing-library/reactto more intelligently wait for the expected condition instead of just using a timer. However, to please TypeScript I had to manually add types for this package, which I did in a way that didn't please our rule for snakeCase filenames. So to get around that I had to add an exception for a new folder I made with the name@testing-library. Alternatively I could of course have renamed the folder, but I felt this was an ok exception.errortag will be applied to all requests. As far as I can see we do not use this information for anything in 6.8, so hopefully this shouldn't be an issue. If however, I have overlooked something, we need this to be fixed before we can use Node.js 12.What's next?
As described above, this PR isn't meant to be merged into
6.8at this time. For now we'll keep it around in case we need Kibana 6.8 to run on Node.js 12. To make it easier to keep this PR up to date with changes in 6.8 (not that we expect many, but anyhow), we could land a few of the commits in 6.8 not directly related to the Node.js upgrade which should make the delta and hence the maintenance cost smaller.We should be able to land the following commits right now:
@testing-library/react359b65b