site-kit-wp
site-kit-wp copied to clipboard
Update Node.js version
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
nodeto18seems to require upgrading other dependencies. Rather than list them here I've linked to a proof-of-concept PR: #7881 (seepackage.jsonin the diff). - Upgrading most Jest/
@wordpress/*/webpack/storybook packages to their latest versions. - Using #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
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
This is a fairly urgent upgrade now, as 14 completely lost all kind of support (even security patch) a month ago. cc @aaemnnosttv
@kuasha420 @aaemnnosttv Thanks for the flag! I can add this to Sprint 103; should this be upgraded from a P2?
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 😄
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! 😄
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.
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.
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 😄