I want to resurrect @MariaAga’s effort to upgrade our frameworks.
The plan is to upgrade:
webpack to v5
react to v17
I know that it may require additional upgrades and currently I want to understand the dependency tree of such a change.
The idea is to find the smallest possible chunk to be a first step in the chain.
I do know that there is a problem with our snapshot tests, so as a first step I would encourage all plugin maintainers to stop relying on those tests and either remove them completely, or change them to a different approach.
@ekohl If you already know about constraints related to our platform, I would really appreciate if you could tell me about those.
We may take smaller steps, for example move to webpack 4, if it will make the deltas more contained.
This would make me aim for NodeJS 18 since we can update the packages on Ubuntu 22.04, but I’d be hesitant to downgrade on Debian 12.
From there I’d look at how to get there. As you said: we must upgrade webpack. That’s probably the first step. I don’t know if doing it in two steps is easier than leapfrogging from 3 to 5. Since we’re going to be breaking things, it may also be viable to just break it hard and fix any fallout but I can’t judge that now.
And the hard part is updating all dependencies. We’ve done a poor job of that in the past and there’s a lot of catching up to do.
If you’ve got a large number of enzyme unit tests (> 1200 in our case) but you still want to switch to react 18 right now and migrate to react-testing-library progressively you can do it with this method (inspired by this article)
I’d be very much in favor of killing snapshots. I think they’re a poor way to test and give little to no confidence. In Using react-testing-library in Katello this was discussed and I’m glad Katello is using a better way. This should be the common way for Foreman and all plugins. Don’t spend any time on fixing snapshot support, but really test the code. It’ll be better for quality and the git logs.
Im trying to make sure every component that we refactor to pf4 is tested with react-testing-library, but that would take a lot of time and I think we should have react 16 in our snapshot testing until then (and react 17+ everywhere else)
exactly, this is a workaround so we can start using React 17/18, and keep the Enzyme (not just snapshots) tests running. @ekohl there are also integration JS tests that are using Enzyme and cover a similar philosophy like react testing library.
I was just talking about killing snapshots. To be clear: I was talking about methodology, not technology. And I realize it’s easier said than done.
This is an area we can divide the work between teams. If we have guidance on how it should look, individual plugin owners can work on converting their plugins.
I dont think we should spend time writing test to components that we want to replace as it seems like double work. But I agree we can help write some guidelines on testing for plugins.
Something I recall being an issue, and I ran into exploring before is not only is it about what version of NodeJS is available, it’s about what version of NPM is bundled with each version:
From what I see webpack 5 requires node v10+, which is good, since we already use at least node 12.
I didn’t see direct requirement to change npm version, unless I am missing something.
@ekohl I understand the concern to upgrade the automatic testing to GH actions and Jenkins pipelines. Those changes will give us more flexibility to test the changes much faster. I am not convinced yet that it is a must-have in order to switch to webpack5, so I think we could leave this change to the next steps in the process.
@MariaAga, is there any component that will not run on npm < 8 after the upgrade to webpack 5? I want to understand when we should do the npm upgrade.
I think that at least at this moment, the next immediate step would be to change webpack to v5.
I don’t see any additional dependencies that have to be upgraded for this to happen.
My plan would be to downgrade the machine to NPM 6.14 and Node 12 (lowest possible), upgrade webpack to 5 and see if I can start the system and run tests on this setup. If it’s possible - I think it would be a good candidate for the first step.
@ekohl , let’s grab some time to talk about the test infrastructure unification and see when these steps have to happen and what benefits they bring to the table.
@ehelms , I am still not convinced at which step we want to upgrade our npm/node dependencies, but it looks like currently we have to postpone this change unless we want to stop supporting EL8’s default setup. EL8 defaults to npm 6, but the package-lock.json structure changes between npm 6 and 8, so if we modify our lock file, it will break EL8 builds.
Do we have to use the default npm version, or is it possible to use npm 8 / use node >= 16 (which already comes with npm 8)?
I wanted to update to npm 8 before doing the webpack update as there are overrides in npm 8 which might be helpful during the upgrade (overrides | package.json | npm Docs), and the lockfile v2 (which is created with npm 8) is backwards compatible with npm 6. But it is not required.
Just wanted to make it clearer that React upgrade isn’t blocking us from Webpack upgrade,
we can still upgrade Webpack if we want before upgrading React, but we definitely should be moving forward also with React.
+1 for removing snapshot testing, but we still have a lot of unit tests and integration tests that are using Enzyme. Since I don’t see us re-writing most those tests in the near future,
we can follow the approach mentioned here:
The TLDR is that we can have two jest.config.js
run jest twice : on --config jest.config.js then on --config jest2.config.js
In the example they use .spec2.js for new test’s files using react-testing-library (and react 18 under the hood)
They keep their old .spec.js for old tests using enzyme (and react 17 under the hood) and migrate them progressively
I think the primary thing I would want out of it is a simple way to select the correct versions of NodeJS (and Ruby) for a certain Foreman version. Today you need to spend a lot of effort in many repositories.
I don’t have a pony in the race much, but my thinking is, that this effort might be much easier when we would directly switch to webpack less javascript building, I know @Ron_Lavi already talked about it somewhere.
Given how much effort this upgrade is, I believe it would be better to switch to import maps right away.
IMHO we are too invested in react to do importmaps. Specifically we have react components composition, where we have sub-components coming from different plugins. I am not entirely sure that this would work well with sourcemaps, although I think it can be a nice addition to managing common dependencies. From reading DHH’s ideas around importmaps, he’s targeting JS-light applications for it and I don’t think Foreman still qualifies as such.
I do think that RedHat’s microfrontends framework can be a quite good helping tool, since it gives us exactly what we are missing: a composable client. In order to test it, the first step would be to upgrade webpact and react. Once we are there, we can either decide to ditch the whole webpack, or to continue to consume Javascrpt built by webpack with or without the microfrontends framework.
That is the goal of the current effort.
