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

Update Node.js version #6026

Open
nfmohit opened this issue Oct 19, 2022 · 7 comments
Open

Update Node.js version #6026

nfmohit opened this issue Oct 19, 2022 · 7 comments
Labels
P1 Medium priority Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Oct 19, 2022

Feature Description

We are currently running on a roughly older LTS of Node (14) and we should explore updating to the newer LTS (18) to benefit from the performance improvements and npm v7.

This came up while working on #5345 where https://github.com/browserslist/update-db isn't compatible with npm versions < 7.

This issue should include updating the browserslist-update-action to the latest version, see: https://github.com/google/site-kit-wp/pull/5883/files#r1004133133.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The version of Node we require (using NVMRC, etc.) should be changed to Node 18
  • Site Kit should build, run tests, etc. using Node 18
  • All GitHub Actions should run when the version of Node is upgraded.

Implementation Brief

  • Update the version of Node to 18 in .nvmrc
  • Upgrading node to 18 seems to require upgrading other dependencies. Rather than list them here I've linked to a proof-of-concept PR: Update Node version to 18. #7881 (see package.json in the diff).
  • Upgrading most Jest/@wordpress/*/webpack/storybook packages to their latest versions.
  • Using Update Node version to 18. #7881 as a starting point, ensure:
    • All Jest tests pass
    • Storybook tests work + pass
    • E2E tests work/pass

Test Coverage

  • Ensure all tests pass.

QA Brief

  • Plugin should be thoroughly smoke-tested as this adjusts the build process of the plugin.

Changelog entry

@tofumatt
Copy link
Collaborator

A note here about something else we can improve in the code when we upgrade Node: https://github.com/google/site-kit-wp/pull/6514/files#diff-f4bce83c408f8dc8922d771b6e6148ca21c6b980d139be6df4cc15b4505b1f0fR27-R42

@kuasha420
Copy link
Contributor

This is a fairly urgent upgrade now, as 14 completely lost all kind of support (even security patch) a month ago. cc @aaemnnosttv

@mxbclang
Copy link

mxbclang commented Jun 2, 2023

@kuasha420 @aaemnnosttv Thanks for the flag! I can add this to Sprint 103; should this be upgraded from a P2?

@aaemnnosttv
Copy link
Collaborator

I think we can make it a P1 but it's not critical – Node is only something we use for builds and local development. The plugin still supports PHP 5.6 (just like WP core) which has long been EOL much longer 😄

@tofumatt
Copy link
Collaborator

I've left this as a 19 because it will likely involve E2E work and other unknowns, but I spent some time getting Node 18 builds working with many WordPress upgrades and a React upgrade—it does build in-browser and works without issue so far! 😄

@tofumatt tofumatt removed their assignment Nov 16, 2023
@techanvil techanvil self-assigned this Nov 17, 2023
@techanvil
Copy link
Collaborator

techanvil commented Nov 17, 2023

Hey @tofumatt, while I commend the spirit of the IB here, I must flag some concerns with it.

From a high level view, it's doing a huge upgrade of our core packages - React v16 -> v18, many @wordpress packages e.g. @wordpress/data v4 -> v9, Webpack v4 -> v5, Puppeteer v10 -> v21. Although you've had some success getting it to build and run locally, there are still so many untested waters here that if we were to proceed, I'd be very hesitant to do so on the basis of an estimate of 19, this feels like one of those issues that could easily balloon or run into an unforeseen intractable issue.

Ideally, we should spec some or all of these upgrades separately - we do have various related issues - but with the interdependencies involved, it's an attractive proposition to do a big upgrade if we can pull it off. But, there are additional questions about the viability of doing so.

Looking into the detail a bit more and taking a look at the draft PR, I notice that trying to run npm ci with the latest Node 18 and the version of NPM that comes with it, it fails due to the react peer dependency for @material-ui/core which requires React 16 or 17.

image

It turns out there also a number of packages in the @material scope with a peer dependency for React 16/17 that would need to be updated.

Digging into it, it turns out that there's no @material-ui/core version that has a peer dependency on React 18. Rather, we'd need to upgrade to Material UI v5 for React 18 support (or otherwise replace use of @material-ui/core).

These peer dependency issues are a useful pointer for potential problems with the upgrade to React 18.

If I ignore the peer dependencies and npm ci anyway - this is possible by downgrading to NPM v7, or by using the --legacy-peer-deps flag - the modules will install, and Site Kit will build, but it doesn't work properly for me. On the Dashboard, the GoogleChart component throws an error which isn't caught and the whole plugin area goes blank. The Settings page is non-functional, the module sections don't open and the tabs are unresponsive.

Storybook also doesn't build for me, and we haven't really considered our GitHub workflows in any depth. This all adds to the degree of uncertainty about this upgrade.

Taking all the above into account, it seems we should rethink this one a bit. When you were upgrading the various modules, was it necessary to update all the way to React 18, or could there be an upgrade path that involves less of a wholesale update to so many of our core modules?

Alternatively, maybe we could upgrade our Node version more incrementally. Interested to hear your thoughts.

@techanvil techanvil assigned tofumatt and unassigned techanvil Nov 17, 2023
@tofumatt
Copy link
Collaborator

Odd, I was able to get a dashboard to build and run for me, but I will look into some of your issues to see if I can reproduce them 👍🏻

So, the issue is that basically we're just super-out-of-date for lots of dependencies and a lot of them are interconnected.

I did need to use --legacy-peer-deps to install for now because I didn't want to update everything, but we are using quite old versions of a lot of libraries.

Fair point about the estimate, which I think we could increase, but I think this is one of those issues that is inescapably big and needs to be a big change at once. I can check into upgrading to Node 16, but if we need to spend a lot of time upgrading I'd rather do it to the latest version of everything, I think it'll be painful either way 😅

I'll take a look through these build issues and see if I can make the drastic approach work. If not, I'll scale it back.

I know we have separate issues for things like React 18 and WP packages, but doing it en-masse strikes me as better as there's so much inter-dependence and it's a "big scary upgrade" either way 😄

@tofumatt tofumatt removed their assignment May 15, 2024
@techanvil techanvil added P1 Medium priority and removed P2 Low priority labels Nov 20, 2024
@eclarke1 eclarke1 added the Team M Issues for Squad 2 label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

7 participants