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

[6.8] Upgrade from Node.js 10 to 12 #100963

Closed
wants to merge 24 commits into from
Closed

[6.8] Upgrade from Node.js 10 to 12 #100963

wants to merge 24 commits into from

Conversation

watson
Copy link
Contributor

@watson watson commented May 29, 2021

68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f78467054376c4d56354d6b7171304536594d2f67697068792e676966

Depends on

Known breaking changes

  • FreeBSD 10 is no longer supported. build: stop supporting FreeBSD 10 nodejs/node#22617
  • CentOS 6 is no longer supported (this isn't officially stated anywhere, but according to our internal testing that's the case)
  • 3rd party plugin developers who use native modules will have to recompile those modules to work on the new Node.js version.

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

  • This PR is not meant to be merged into 6.8 at 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 custom 6.8-nodejs12 branch.
  • Since this is a major change to the 6.8 code, and since the 6.8 branch doesn't have as good test-coverage as 7.x, please be extra vigorous and do some manual testing, as relying on CI being green could give a false sense of security 🙏
  • I had to upgrade the typescript dependency 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.
  • With the upgrade to Node.js 12 comes support for TLSv1.3 and by default the dropped support for older TLS versions. I re-enabled support for those old TLS versions to ensure we don't break anything by adding the --tls-min-v1.0 flag to bin/kibana. I think that's the only place I need to add it. Update: Now also added to bin/kibana.bat (thanks to @mieciu for spotting this)
  • The upgrade of @types/node also meant that a few changes needed to be made to our source code to please the types.
  • A few internal changes in Node.js 12 related to timings of async events meant that a few of our tests failed due to race conditions. The easiest fix was to use the module @testing-library/react to 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.
  • Another change in Node.js 12 meant that the internal structure of Arrays in JavaScript changed slightly so that the internal ordering of elements in the array isn't as they were in Node.js 10. This broke a few of our tests that was relying on the internal logic for element order when sorting. I simply updated those to match the new behavior.
  • The version of hapi used in 6.8 has an issue with Node.js 12 which means that the hapi error tag 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.8 at 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:

  • "Bump fsevents and node-sass to versions that support Node.js 12" 6b83f9d
  • The parts of "Fix broken tests related to Node.js 12 upgrade" that's related to @testing-library/react 359b65b
  • "Upgrade typescript from v3.0.3 to v3.1.8" 6896bed
  • "[test] fix tests related to timings" 2f3a5ab
  • "Never manually emit errors on streams" 063a8a0
  • "Backport Node.js 12 related changes from 7.x" 5129dc7

@watson watson added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels May 29, 2021
@watson watson self-assigned this May 29, 2021
@watson watson force-pushed the bump-node branch 2 times, most recently from 882dbd3 to 0ecec1c Compare June 3, 2021 07:02
@watson watson force-pushed the bump-node branch 5 times, most recently from 17d689b to 75026e0 Compare June 8, 2021 11:33
@watson watson marked this pull request as ready for review June 10, 2021 07:51
@watson watson requested a review from a team June 10, 2021 08:01
@watson watson requested review from kobelb and a team June 10, 2021 08:03
@watson watson removed the release_note:skip Skip the PR/issue when compiling release notes label Jun 10, 2021
@jbudz
Copy link
Member

jbudz commented Jun 10, 2021

Hmm - I can't get this to pass:

/home/jon/dev/kibana/node_modules/typescript/bin/tsc --pretty true --project /home/jon/dev/kibana/build/kibana/tsconfig.json
build/kibana/src/plugins/usage_collection/target/types/server/plugin.d.ts:1:141 - error TS1005: ';' expected.                                                                                                      
                                                                                                                                                                                                                   
1 import type { PluginInitializerContext, CoreSetup, CoreStart, Plugin, ElasticsearchClient, SavedObjectsClientContract, KibanaRequest } from 'src/core/server';                                                   
                                                                                                                                              ~~~~~~~~~~~~~~~~~                                                    
                                                                                                                                                                                                                   
build/kibana/src/plugins/usage_collection/target/types/server/plugin.d.ts:2:13 - error TS1005: '=' expected.                                                                                                       
                                                                                                                                                                                                                   
2 import type { Collector, CollectorOptions, UsageCollectorOptions } from './collector';                                                                                                                           
              ~                                                                                                                                                                                                    
                                                                                                                                                                                                                   
build/kibana/src/plugins/usage_collection/target/types/server/plugin.d.ts:2:73 - error TS1005: ';' expected.                                                                                                       
                                                                                                                                                                                                                   
2 import type { Collector, CollectorOptions, UsageCollectorOptions } from './collector';                                                                                                                           
                                                                          ~~~~~~~~~~~~~       

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.

@watson
Copy link
Contributor Author

watson commented Jun 10, 2021

Maybe a yarn kbn clean will fix it?

Copy link
Member

@jbudz jbudz left a 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

@kobelb
Copy link
Contributor

kobelb commented Jun 30, 2021

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

@dmlemeshko dmlemeshko requested a review from marius-dr July 1, 2021 13:42
@watson watson changed the title [6.8] Upgrade Node.js from 10.24.1 to 12.22.1 [6.8] Upgrade from Node.js 10 to 12 Jul 6, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@liza-mae
Copy link
Contributor

OOD

@liza-mae liza-mae closed this Jul 20, 2022
@watson watson deleted the bump-node branch July 26, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants