Resurrection of the client-side infrastructure upgrade effort

Development
#1

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.

3 Likes
Foreman Community Demo #121
#2

My perspective is very much on the infra side, less on the actual code side. It must also happen, but others have a better view on that.

I would actually start with step 0: identify which plugins you want to verify. I started Foreman landscape so that would probably be a good start.

You didn’t mention the NodeJS version, but that must also be updated. That means you want working CI on it, so you can verify your changes. We have a lot of different ways of configuring CI. I started a repo with reusable GitHub Actions, which make it much easier to change the NodeJS. There’s Verify it works with all plugins · Issue #1 · theforeman/actions · GitHub with a list of TODO items. On the Jenkins side I started to update as well in Convert plugin testing to pipelines by ekohl · Pull Request #11 · theforeman/jenkins-jobs · GitHub but have yet to finish that.

Only with that in place I’d feel comfortable moving forward.

The next step is to determine the new supported NodeJS version(s). If we look at what the distros have available where we’d be running in the future:

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.

#3

Thanks @Shimon_Shtein !

I saw this comment the other day in Support for React 18 · Issue #2524 · enzymejs/enzyme · GitHub

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)

For React 17 there’s an unofficial adapter for Enzyme: GitHub - wojtekmaj/enzyme-adapter-react-17: Unofficial adapter for React 17 for Enzyme.

but for React 18 it will definitely break… but with Support for React 18 · Issue #2524 · enzymejs/enzyme · GitHub we might not need to replace all of our tests that used Enzyme.

#4

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.

#5

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)

#6

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.

#7

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.

#8

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.

#9

Agreed that we don’t need to invest in deprecated things, but instead focus on setting the current best practice and work to making that happen.

I’ve already said to @Shimon_Shtein that I think this will span multiple releases, and that’s ok. Rome wasn’t built in a day.

#10

opened Refactor #36119: Remove snapshot tests - Katello - Foreman for Katello :+1:

#11

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:

  • EL 8 / 9:
    • NodeJS 14 → NPM 6.14 (EL 8 only)
    • NodeJS 16 → NPM 8.19
    • NodeJS 18 → NPM 8.19
  • Debian 12
    • NodeJS 18 → NPM 9.2
  • Ubuntu 22.04
    • NodeJS 12 → NPM 8.5
1 Like
#12

I started updating to npm 8, but didn’t have the time to continue here: feat(root): update lock files to v2 / npm 8 by MariaAga · Pull Request #404 · theforeman/foreman-js · GitHub

#13

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.

#14

@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)?

#15

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.

#16

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

And this will allow us to migrate to React 17/18

#17

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.

Note that for Path to Ruby 3.0, 3.1, EL9 and Ubuntu 22.04 we’ll want the same thing.

#18

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.

More details on the new stack in Rails 7.0, if anyone would be interested: Ruby on Rails — Rails 7.0: Fulfilling a vision

1 Like
#19

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.

1 Like
#20

First version of the PR is out: Switch foreman to webpack 5 by ShimShtein · Pull Request #9646 · theforeman/foreman · GitHub
It runs, but the build time increased, so it can cause a timeout on cold dev server startup.
Also I can see that the react-app package is duplicated multiple times:

image
image1926×927 508 KB

Next steps would be:

  • Go over existing plugins and decide what’s still needed and what things are deprecated.
  • Switch to webpack 5 in foreman-js package
2 Likes