-
Notifications
You must be signed in to change notification settings - Fork 482
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
`jaeger-ui` currently uses build tooling based on `react-scripts` (née CRA) 2.1.3, which dates back to early 2019. Since `react-scripts` is an opinionated wrapper around other tooling such as `jest` and `webpack`, it in turn keeps those locked on outdated versions. There are a few options I considered here: * **Do nothing** - ignore the outdated `react-scripts` and try to upgrade other things without touching it. This is what I tried first, when I tested out migrating some tests to `@testing-library` from `enzyme`. Unfortunately, I found that the outdated `jest` / `jsdom` combo precluded effective use of that library as some events and APIs were simply not available. * **Try to update `react-scripts`**. Unfortunately, there is a problem here - although `react-scripts` is intended to be a zero-configuration wrapper around CRA tooling, the project attempts to modify its settings via two different packages (`react-scripts-rewired` and `customize-cra`) that power a custom buildscript which attempts to manipulate the webpack configs normally abstracted away by `react-scripts`. Given that all this hassle is done only to perform some mundane tasks like integrating with AntD's Less styles, or making the build step respect the project's ESLint config, I am not sure if staying with `react-scripts` and trying to customize it with a bag of tricks is a sustainable approach. * **Eject the `react-scripts` and work with that**. `react-scripts` has an escape hatch that dumps the build tooling it uses internally in the project, should you decide that you want to roll your own build. The problem I saw with this is that this simply leaves us with a bunch of messy webpack config files that we then need to maintain going forward. * **Migrate to a different build tool**. There are some promising tools out there that try to strike a reasonable balance between ease of configuration and use and extensibility, such as Vite and Parcel. I ended up settling on this approach after exhausting the above options, using Vite as the bundler of choice. I also gave Parcel a try but unfortunately I ran into issues trying to get it to transpile the AntD Less stylesheet, so I ended up not exploring it further for now. So, per the above points, this PR reconfigures the build step to use Vite rather than `react-scripts`. There are a few caveats I ran into: * Vite by default doesn't interpret JSX tags in .js files. This proved surprisingly difficult to override, so I renamed the few affected files to `.tsx` (if migrating didn't require sweeping changes) or `.jsx` otherwise. * Consuming the plexus build resulted in some errors due to ambiguous exports, so I reconfigured it to output ESM rather than UMD. * The abandoned `react-dimensions` package, used to automatically size/resize the scatterplot diagram on the trace search page, threw an error at runtime due to gratuitous use of `this`. I replaced it with a simple hooks-based implementation. * The `rc-menu` component used internally by AntD would crash the entire page due to it merrily using `require()` at runtime and assuming it would be present in the environment. I ended up just providing a basic `require()` stub, since it was only using it to load a `MutationObserver` polyfill. Fortunately, the problematic code was removed in newer versions of the component. * The customized `react-scripts` setup used a Babel plugin to automatically import styles whenever an AntD React component was imported. This also seems doable with Vite; for simplicity and to reduce the number of build deps, I haven't configured it though. If the bundle size impact is considered prohibitive, this can be revisited. Overall, I am cautiously optimistic about Vite - the developer experience seems significantly better as the HMR server is notably snappier, at least on my machine. Most of the above issues stemmed from the fact that many dependencies are outdated (and therefore more likely to contain code that doesn't play that well with present tooling). This change also allowed me to migrate the test suite from Jest v23 to Jest v28 with minimal changes. Jest v29 unfortunately doesn't yet work as it doesn't play well with the old TypeScript version currently used by the project. It may be worth noting that the `react-scripts` build step would also run ESLint at build time. This is doable with Vite as well, I'm just not sure if it's worth it, since the linter is already executed separately in continuous integration. It therefore may not be necessary to have it slow down the build. Signed-off-by: Máté Szabó <mszabo@fandom.com>
- Loading branch information
1 parent
adf7e8e
commit 1ba2345
Showing
43 changed files
with
1,803 additions
and
6,217 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| module.exports = { | ||
| globals: { | ||
| __REACT_APP_GA_DEBUG__: false, | ||
| __REACT_APP_VSN_STATE__: false, | ||
| __APP_ENVIRONMENT__: false, | ||
| }, | ||
| }; |
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
File renamed without changes.
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
Oops, something went wrong.