Migrate build tooling to Vite #1212

mszabo-wikia · 2023-02-25T03:07:06Z

Which problem is this PR solving?

Contributes towards Typescript version is outdated #818, Help needed with upgrading dependencies #1199

Short description of the changes

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.

mszabo-wikia · 2023-02-25T03:07:43Z

Any feedback is welcome. If there is something I'm not, it's an expert in modern frontend.

mszabo-wikia · 2023-02-25T03:29:51Z

packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx

@@ -455,7 +457,7 @@ export default connect(
  mapStateToProps,
  mapDispatchToProps
 )(
-  reduxForm({


These type errors popped up after removing react-scripts. What's weird about them is that they seem valid—the prop types of MonitorATMServicesViewImpl don't actually match what's coming in from the HOC, so I am not sure why these were not failing before.

mszabo-wikia · 2023-02-25T03:30:47Z

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.tsx

@@ -16,15 +16,14 @@



This file was apparently using Flow types, so I ended up converting it to TS instead of just renaming to .jsx.

codecov · 2023-02-25T03:45:57Z

Codecov Report

Base: 95.39% // Head: 95.40% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (15f692a) compared to base (adf7e8e).
Patch coverage: 77.58% of modified lines in pull request are covered.

❗ Current head 15f692a differs from pull request most recent head c1eeaf7. Consider uploading reports for the commit c1eeaf7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
+ Coverage   95.39%   95.40%   +0.01%     
==========================================
  Files         243      244       +1     
  Lines        7571     7753     +182     
  Branches     1898     2018     +120     
==========================================
+ Hits         7222     7397     +175     
- Misses        342      355      +13     
+ Partials        7        1       -6

Impacted Files	Coverage Δ
packages/jaeger-ui/src/components/App/index.jsx	`100.00% <ø> (ø)`
...eger-ui/src/components/DeepDependencies/traces.tsx	`100.00% <ø> (ø)`
...s/jaeger-ui/src/components/DependencyGraph/DAG.jsx	`100.00% <ø> (ø)`
...omponents/DependencyGraph/DependencyForceGraph.jsx	`89.28% <ø> (ø)`
...jaeger-ui/src/components/DependencyGraph/index.jsx	`100.00% <ø> (ø)`
...r-ui/src/components/SearchTracePage/SearchForm.jsx	`91.61% <ø> (ø)`
...jaeger-ui/src/components/SearchTracePage/index.jsx	`88.09% <0.00%> (ø)`
...ackages/jaeger-ui/src/constants/default-config.tsx	`50.00% <ø> (-16.67%)`	⬇️
packages/jaeger-ui/src/index.tsx	`0.00% <0.00%> (ø)`
...components/SearchTracePage/SearchResults/index.tsx	`83.01% <66.66%> (ø)`
... and 91 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mszabo-wikia · 2023-02-25T13:52:31Z

package.json

-      "**/react-scripts/**",
-      "**/react-app-rewired",
-      "**/react-app-rewired/**"
+      "**/parse5"


This one is here because it contains type definitions with syntax that TypeScript 3.5 cannot comprehend. It could conceivably be removed once TS is upgraded.

yurishkuro · 2023-02-25T22:27:57Z

@mszabo-wikia I will need to look deeper into Vite, but so far seems like a reasonable choice.

However, do you think it's possible to partition the changes in this PR such that the changes to the source code (typing, renaming) could be committed separately, before making changes to the build?

yurishkuro · 2023-02-25T22:30:14Z

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx

@@ -0,0 +1,120 @@
+// Copyright (c) 2017 Uber Technologies, Inc.


It would be good to undo this file's change and redo it such that Git would recognize the move, otherwise it's impossible to see what changed.

yurishkuro · 2023-02-25T22:32:34Z

packages/jaeger-ui/src/utils/tracking/ga.tsx

-  const isTest = process.env.NODE_ENV === 'test';
+  const isProd = __APP_ENVIRONMENT__ === 'production';
+  const isDev = __APP_ENVIRONMENT__ === 'development';
+  const isTest = __APP_ENVIRONMENT__ === 'test';


nit: I would prefer to future-proof this a bit and isolate all env var access to a util module, so that we could change it in one place instead of many files. The "isolate" part can be a standalone PR merged first.

This is so that Vite correctly renders JSX inside these. Signed-off-by: Máté Szabó <[email protected]>

SearchResults was using Flow types, so it was easier to convert it to TS rather than plain JS. The index component was trivially upgradable to TS. Signed-off-by: Máté Szabó <[email protected]>

This package is abandoned and doesn't play well with Vite, because one of its dependencies uses `this` to access the global context in a way that apparently fails in a module context. Replace this dependency with a simple hooks-based implementation. Signed-off-by: Máté Szabó <[email protected]>

These type errors popped up after removing react-scripts. What's weird about them is that they seem valid—the prop types of MonitorATMServicesViewImpl don't actually match what's coming in from the HOC, so I am not sure why these were not failing before. Signed-off-by: Máté Szabó <[email protected]>

This needs to be an import since Vite uses ES modules. Signed-off-by: Máté Szabó <[email protected]>

Signed-off-by: Máté Szabó <[email protected]>

The current output resulted in "ambiguous export" errors when using Vite. Signed-off-by: Máté Szabó <[email protected]>

These will be used when running without `react-scripts`. Signed-off-by: Máté Szabó <[email protected]>

Signed-off-by: Máté Szabó <[email protected]>

mszabo-wikia · 2023-02-26T18:22:12Z

@yurishkuro Thanks for the quick review. I split up the changeset so that it's hopefully easier to review :)

yurishkuro · 2023-02-26T23:27:46Z

I split up the changeset so that it's hopefully easier to review :)

Do you mean you split the in the commits only? I was referring to making independent PRs, because some of the refactoring of types etc. seems like it would be applicable to the current build system as well, so I would prefer to merge them independently, and only they have a PR that changes the build system. This way it will be easier to revert changes should the need arise.

mszabo-wikia · 2023-02-27T03:00:41Z

Ah sorry, I misunderstood. Absolutely, I'll try and split out those standalone parts into separate PRs.

## Which problem is this PR solving? * Split from #1212 ## Short description of the changes This is to ease an eventual migration to Vite, since Vite by default only supports JSX in files with the .jsx or .tsx extensions, and this is surprisingly hard to override. Update the Prettier and ESLint configurations accordingly. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? Split from #1212 ## Short description of the changes Plexus currently outputs a Universal Module Definition, which seems to cause issues during the Vite build as it results in "ambiguous exports". Have Plexus output ESM instead to avoid the issue. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Split from #1212 ## Short description of the changes As with #1218, convert SearchResults and the root index component to use TS and the .tsx file extension rather than plain .js. TS was chosen here since SearchResults was already using Flow types, and index is trivially convertible. Adding `@types/react-form` as a new dev dependency to provide typings for `redux-form` in `SearchResults` in turn revealed type errors in the usage of `redux-form` in the `ServicesView` component, so update that component accordingly. Likewise, the conversion from Flow to TS revealed some typing errors in child components. Also update the list of JS files to be included in the TS linting process - since we now have two new TS files to be built and linted, the JS files that these import in turn need to be explicitly listed in tsconfig.lint.json, otherwise `tsc` complains that they are not included in the build. Signed-off-by: Máté Szabó <[email protected]>

) ## Which problem is this PR solving? - Split from #1212 ## Short description of the changes This package is abandoned and doesn't play well with Vite, because one of its dependencies uses `this` to access the global context in a way that apparently fails in a module context. Replace this dependency with a simple hooks-based implementation. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Split from #1212 ## Short description of the changes Create a wrapper module for accessing globals injected by the build system. This makes it easier to rename or adjust those later. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Split from #1212 ## Short description of the changes This needs to be an import for an eventual build system switch, since Vite uses ES modules. It also helps make things more consistent. The UI package version in the About Jaeger dropdown still shows up. --------- Signed-off-by: Máté Szabó <[email protected]>

mszabo-wikia · 2023-03-01T23:56:01Z

Going to close this since most changes have been split out. I hope to have a PR up for the final build system switch momentarily.

Switch the project to build using Vite, per the considerations outlined in jaegertracing#1212. Notable changes are: * `react-scripts` would provide some built-in Jest transforms and mocks under the hood, which need to be defined explicitly now. * Build-time injected constants no longer utilize `process.env`. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Contributes towards #818, #1199 ## Short description of the changes Switch the project to build using Vite, per the considerations outlined in #1212. Notable changes are: * `react-scripts` would provide some built-in Jest transforms and mocks under the hood, which need to be defined explicitly now. * Build-time injected constants no longer utilize `process.env`. ## Testing For this, I spun up a local `all-in-one` jaeger instance and fed it some test data with `microsim`. I updated the `jaeger-ui` submodule reference for this local jaeger checkout to point to this branch to test the behavior of the production build. I then navigated around looking for errors on the pages or the console. <img width="1512" alt="Screenshot 2023-03-02 at 01 07 39" src="https://user-images.githubusercontent.com/2721291/222296252-539ac6e0-15b5-49d6-943c-e437b852704e.png"> --------- Signed-off-by: Máté Szabó <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

* Split from jaegertracing#1212 This is to ease an eventual migration to Vite, since Vite by default only supports JSX in files with the .jsx or .tsx extensions, and this is surprisingly hard to override. Update the Prettier and ESLint configurations accordingly. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? Split from jaegertracing#1212 ## Short description of the changes Plexus currently outputs a Universal Module Definition, which seems to cause issues during the Vite build as it results in "ambiguous exports". Have Plexus output ESM instead to avoid the issue. Signed-off-by: Máté Szabó <[email protected]>

- Split from jaegertracing#1212 As with jaegertracing#1218, convert SearchResults and the root index component to use TS and the .tsx file extension rather than plain .js. TS was chosen here since SearchResults was already using Flow types, and index is trivially convertible. Adding `@types/react-form` as a new dev dependency to provide typings for `redux-form` in `SearchResults` in turn revealed type errors in the usage of `redux-form` in the `ServicesView` component, so update that component accordingly. Likewise, the conversion from Flow to TS revealed some typing errors in child components. Also update the list of JS files to be included in the TS linting process - since we now have two new TS files to be built and linted, the JS files that these import in turn need to be explicitly listed in tsconfig.lint.json, otherwise `tsc` complains that they are not included in the build. Signed-off-by: Máté Szabó <[email protected]>

…egertracing#1223) ## Which problem is this PR solving? - Split from jaegertracing#1212 ## Short description of the changes This package is abandoned and doesn't play well with Vite, because one of its dependencies uses `this` to access the global context in a way that apparently fails in a module context. Replace this dependency with a simple hooks-based implementation. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Split from jaegertracing#1212 ## Short description of the changes Create a wrapper module for accessing globals injected by the build system. This makes it easier to rename or adjust those later. Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Split from jaegertracing#1212 ## Short description of the changes This needs to be an import for an eventual build system switch, since Vite uses ES modules. It also helps make things more consistent. The UI package version in the About Jaeger dropdown still shows up. --------- Signed-off-by: Máté Szabó <[email protected]>

## Which problem is this PR solving? - Contributes towards jaegertracing#818, jaegertracing#1199 ## Short description of the changes Switch the project to build using Vite, per the considerations outlined in jaegertracing#1212. Notable changes are: * `react-scripts` would provide some built-in Jest transforms and mocks under the hood, which need to be defined explicitly now. * Build-time injected constants no longer utilize `process.env`. ## Testing For this, I spun up a local `all-in-one` jaeger instance and fed it some test data with `microsim`. I updated the `jaeger-ui` submodule reference for this local jaeger checkout to point to this branch to test the behavior of the production build. I then navigated around looking for errors on the pages or the console. <img width="1512" alt="Screenshot 2023-03-02 at 01 07 39" src="https://user-images.githubusercontent.com/2721291/222296252-539ac6e0-15b5-49d6-943c-e437b852704e.png"> --------- Signed-off-by: Máté Szabó <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

mszabo-wikia requested review from everett980, rubenvp8510 and tiffon as code owners February 25, 2023 03:07

mszabo-wikia force-pushed the migrate-build branch from 1ba2345 to a942be4 Compare February 25, 2023 03:27

mszabo-wikia commented Feb 25, 2023

View reviewed changes

mszabo-wikia force-pushed the migrate-build branch from a942be4 to c73622c Compare February 25, 2023 03:32

mszabo-wikia commented Feb 25, 2023

View reviewed changes

yurishkuro reviewed Feb 25, 2023

View reviewed changes

mszabo-wikia force-pushed the migrate-build branch from c73622c to 15f692a Compare February 26, 2023 18:18

mszabo-wikia added 10 commits February 26, 2023 19:20

Use .jsx file extension for JS components

4837581

This is so that Vite correctly renders JSX inside these. Signed-off-by: Máté Szabó <[email protected]>

Convert SearchResults and index components to TS

0e47072

SearchResults was using Flow types, so it was easier to convert it to TS rather than plain JS. The index component was trivially upgradable to TS. Signed-off-by: Máté Szabó <[email protected]>

Avoid use of require() in default-config.tsx

e1dc60b

This needs to be an import since Vite uses ES modules. Signed-off-by: Máté Szabó <[email protected]>

Update injected global names and create a wrapper module

f92dd54

Signed-off-by: Máté Szabó <[email protected]>

Make Plexus output ESM

4cdf940

The current output resulted in "ambiguous export" errors when using Vite. Signed-off-by: Máté Szabó <[email protected]>

Add setup files and mocks for Jest

d352eb1

These will be used when running without `react-scripts`. Signed-off-by: Máté Szabó <[email protected]>

Switch jaeger-ui to build using Vite

2a29c4c

Signed-off-by: Máté Szabó <[email protected]>

Fix Prettier config to extend to .jsx files

c1eeaf7

Signed-off-by: Máté Szabó <[email protected]>

mszabo-wikia force-pushed the migrate-build branch from 15f692a to c1eeaf7 Compare February 26, 2023 18:21

This was referenced Feb 27, 2023

Use .jsx file extension for JS components #1218

Merged

Make Plexus output ESM #1219

Merged

mszabo-wikia mentioned this pull request Feb 27, 2023

Convert SearchResults and index components to TS #1222

Merged

This was referenced Feb 27, 2023

Remove use of react-dimensions package in ScatterPlot component #1223

Merged

Abstract access to injected globals #1224

Merged

mszabo-wikia mentioned this pull request Mar 1, 2023

Avoid use of require() in default-config.tsx #1225

Merged

mszabo-wikia closed this Mar 1, 2023

mszabo-wikia mentioned this pull request Mar 2, 2023

Migrate build tooling to Vite #1226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate build tooling to Vite #1212

Migrate build tooling to Vite #1212

mszabo-wikia commented Feb 25, 2023

mszabo-wikia commented Feb 25, 2023

mszabo-wikia Feb 25, 2023

mszabo-wikia Feb 25, 2023

codecov bot commented Feb 25, 2023 •

edited

Loading

mszabo-wikia Feb 25, 2023

yurishkuro commented Feb 25, 2023

yurishkuro Feb 25, 2023

yurishkuro Feb 25, 2023

mszabo-wikia commented Feb 26, 2023

yurishkuro commented Feb 26, 2023

mszabo-wikia commented Feb 27, 2023

mszabo-wikia commented Mar 1, 2023

		@@ -0,0 +1,120 @@
		// Copyright (c) 2017 Uber Technologies, Inc.

Migrate build tooling to Vite #1212

Migrate build tooling to Vite #1212

Conversation

mszabo-wikia commented Feb 25, 2023

Which problem is this PR solving?

Short description of the changes

mszabo-wikia commented Feb 25, 2023

mszabo-wikia Feb 25, 2023

Choose a reason for hiding this comment

mszabo-wikia Feb 25, 2023

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2023 • edited Loading

Codecov Report

mszabo-wikia Feb 25, 2023

Choose a reason for hiding this comment

yurishkuro commented Feb 25, 2023

yurishkuro Feb 25, 2023

Choose a reason for hiding this comment

yurishkuro Feb 25, 2023

Choose a reason for hiding this comment

mszabo-wikia commented Feb 26, 2023

yurishkuro commented Feb 26, 2023

mszabo-wikia commented Feb 27, 2023

mszabo-wikia commented Mar 1, 2023

codecov bot commented Feb 25, 2023 •

edited

Loading