Migrate build tooling to Vite #1226

mszabo-wikia · 2023-03-02T00:13:56Z

Which problem is this PR solving?

Contributes towards Typescript version is outdated #818, Help needed with upgrading dependencies #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.

codecov · 2023-03-02T00:26:30Z

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.03 🎉

Comparison is base (f5d276d) 95.40% compared to head (dfdb18f) 95.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   95.40%   95.43%   +0.03%     
==========================================
  Files         244      244              
  Lines        7589     7751     +162     
  Branches     1901     2017     +116     
==========================================
+ Hits         7240     7397     +157     
- Misses        349      353       +4     
- Partials        0        1       +1

Impacted Files	Coverage Δ
packages/jaeger-ui/src/utils/constants.tsx	`66.66% <66.66%> (ø)`
...ents/TracePage/TraceStatistics/DetailTableData.tsx	`82.14% <0.00%> (-2.48%)`	⬇️
...racePage/TraceStatistics/generateDropdownValue.tsx	`95.55% <0.00%> (-2.12%)`	⬇️
...mponents/TracePage/TraceStatistics/tableValues.tsx	`93.66% <0.00%> (-1.86%)`	⬇️
...i/src/components/TracePage/TraceSpanView/index.tsx	`93.65% <0.00%> (-1.44%)`	⬇️
packages/jaeger-ui/src/utils/tracking/ga.tsx	`94.44% <0.00%> (-1.34%)`	⬇️
...onents/TracePage/TraceStatistics/MainTableData.tsx	`72.22% <0.00%> (-1.31%)`	⬇️
...acePage/TracePageHeader/SpanGraph/ViewingLayer.tsx	`92.07% <0.00%> (-0.93%)`	⬇️
packages/jaeger-ui/src/selectors/trace.js	`91.37% <0.00%> (-0.80%)`	⬇️
...src/components/TracePage/TraceStatistics/index.tsx	`77.77% <0.00%> (-0.64%)`	⬇️
... and 74 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.

yurishkuro

👏

packages/jaeger-ui/.eslintrc.js

packages/jaeger-ui/index.html

yurishkuro · 2023-03-02T00:43:55Z

packages/jaeger-ui/index.html

+
+      // Workaround some legacy NPM dependencies that assume this is always defined.
+      window.global = {};
+      // Avoid noise from redux-form until https://github.com/redux-form/redux-form/pull/4723 is released.


It would be good to file an issue in this repo to track this and explain what needs to be done once the redux issue is resolved.

yurishkuro · 2023-03-02T00:45:28Z

packages/jaeger-ui/index.html

-      To begin the development, run `npm start` in this folder.
-      To create a production bundle, use `npm run build`.
-    -->
+    <script type="module" src="/src/index.tsx"></script>


This is unexpected. Is the browser supposed to interpret a typescript file?

I also wonder about the /src/ prefix, how that would work with jaeger-query

It gets replaced with a proper asset at build time (https://vitejs.dev/guide/features.html#typescript).

Could you please compare the size of the overall ui-asset bundle after switching to Vite? Add it to the PR description as a benchmarking data point.

Looking at the last CI build from main, the results from CRA are:

File sizes after gzip: 863.25 KB build/static/js/1.2bd86f79.chunk.js 654.32 KB build/static/js/main.c8c51bfd.chunk.js 43.11 KB build/static/css/1.7f7667b5.chunk.css 10.62 KB build/static/css/main.5918f9f5.chunk.css 763 B build/static/js/runtime~main.4a686d48.js

That'd be ~1519 kB of JS (gzipped) and ~54 kB of CSS (gzipped).

For this branch, we have:

build/index.html 5.17 kB build/static/monitor-9000dba4.png 111.77 kB build/static/jaeger-logo-ab11f618.svg 161.92 kB build/static/index-b15a508e.css 352.14 kB │ gzip: 56.11 kB build/static/index-1af2a95e.js 6,008.36 kB │ gzip: 1,642.99 kB

So the JS bundle ends up larger. Not immediately sure what the increase is due to; will add rollup-plugin-visualizer locally to try and hopefully determine that.

One finding: the old build system used babel-plugin-import not just for the antd handling but also for magically optimizing lodash imports. Most places already use lodash/subComponent import style to only import needed code, but some import the entire library. Locally, changing those to all use appropriate subcomponent imports shaved off ~30 kB from the gzipped bundle.

With that, the bundle ends up like this:

It seems the biggest opportunity for saving some bytes would be to lazy-load dependencies that are only needed on specific pages, e.g. plexus, or the pyroscope flamegraph visualizer.

Indeed, lazy loading those would be good. Does it require changing the build to produce several js files?

Looks like this is doable by using dynamic imports for the chunks that we want to lazyload. I tested locally with DAG.jsx by moving the top-level imports for cytoscape and related deps into the componentDidMount callback:

const [{ default: cytoscape }, { default: cydagre }, { default: dagre }] = await Promise.all([ import('cytoscape'), import('cytoscape-dagre'), import('dagre'), ]);

This split out these dependencies from the main chunk and Vite automatically generated separate chunks for them.

For the issue with lodash, we could make a separate PR to convert the few usages where the entirety of lodash is imported into appropriate sub-component imports. Alternatively, it could also be solved by switching it out for lodash-es, which is lodash but built as ES modules, making import { foo } from 'lodash-es' "just work" without requiring a special plugin (it'd only import foo and not the whole library).

Filed #1233 and #1234 to track these.

packages/jaeger-ui/index.html

yurishkuro · 2023-03-02T00:56:35Z

packages/jaeger-ui/src/setupProxy.js

@@ -1,59 +0,0 @@
-// Copyright (c) 2019 Uber Technologies, Inc.


Why don't we need it? IIRC the proxy was used to allow running dev version of the UI on port 3000 while running all-in-one on the official port

Proxying is now configured in vite.config.ts, so this file can go away.

packages/jaeger-ui/src/utils/constants.tsx

yurishkuro · 2023-03-02T01:05:46Z

packages/jaeger-ui/tsconfig.lint.json

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-// This file is necessary because create-react-app (CRA) requires
+// This file is necessary because the build system (Vite) requires
 // isolatedModules to be true but for linting we need it to be false. We run


curious if the new build would help remove the need for this override / split.

I tested it out, but unfortunately got some failures (Vite and most build tools imply isolatedModules), so I decided to not extend the scope of this PR with potential TS changes. But yeah, it's something worth checking separately.

yurishkuro · 2023-03-02T01:07:49Z

packages/jaeger-ui/config-overrides-antd-vars.less

@@ -1,24 +0,0 @@
-@primary-color: #199;


moved to packages/jaeger-ui/vite.config.ts

yurishkuro · 2023-03-02T01:08:07Z

packages/jaeger-ui/jest.global-setup.js

@@ -1,3 +0,0 @@
-module.exports = async () => {


moved to packages/jaeger-ui/test/jest.global-setup.js

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]>

packages/jaeger-ui/index.html

yurishkuro

looks great! I will do some manual testing over the weekend.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-03-07T05:29:26Z

🥳 🥳 🥳

## 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]>

yurishkuro · 2023-08-14T18:03:54Z

packages/jaeger-ui/package.json

+    "@svgr/babel-plugin-transform-svg-component": "^6.5.1",
+    "@svgr/babel-preset": "^6.5.1",


@ mszabo-wikia do you know why/if these were needed?

Yeah, this is likely an artifact. I thought it was still being used by tests for SVG support, but it seems those use babel-plugin-inline-react-svg instead (test/babel-transform.js). So these are probably OK to remove assuming that tests pass without them.

thanks, appreciate your comments

thanks for reaching out! :)

Related to: #1675 and #1672  ## What changes is this PR making? - This PR removes the unused packages from packages/jaeger-ui - The removed packages are: - `@svgr/babel-plugin-transform-svg-component` - `@svgr/babel-preset` - `@types/react-dom` - `@types/react-router-redux` ## Description of the changes - As per the discussion #1226 (comment), and the manual check. Previously, I was reluctant in removing `@types/react-dom` and `@types/react-router-redux` because their corresponding packages are used, but I gave them a test by removing: - run `yarn lint` - no errors - remove these two packages and again run `yarn lint` - no errors - maybe linter is not working correctly, so removed `@types/react-window` - run `yarn lint` - type errors - added back `@types/react-window` and run `yarn lint` - no errors - Thus, these two deps aren't needed ## How was this change tested? - By running yarn lint and yarn test ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <[email protected]>

Related to: jaegertracing#1675 and jaegertracing#1672  ## What changes is this PR making? - This PR removes the unused packages from packages/jaeger-ui - The removed packages are: - `@svgr/babel-plugin-transform-svg-component` - `@svgr/babel-preset` - `@types/react-dom` - `@types/react-router-redux` ## Description of the changes - As per the discussion jaegertracing#1226 (comment), and the manual check. Previously, I was reluctant in removing `@types/react-dom` and `@types/react-router-redux` because their corresponding packages are used, but I gave them a test by removing: - run `yarn lint` - no errors - remove these two packages and again run `yarn lint` - no errors - maybe linter is not working correctly, so removed `@types/react-window` - run `yarn lint` - type errors - added back `@types/react-window` and run `yarn lint` - no errors - Thus, these two deps aren't needed ## How was this change tested? - By running yarn lint and yarn test ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <[email protected]>

mszabo-wikia requested review from everett980, rubenvp8510 and tiffon as code owners March 2, 2023 00:13

yurishkuro reviewed Mar 2, 2023

View reviewed changes

mszabo-wikia force-pushed the migrate-to-vite branch from 260d9fb to f2921bc Compare March 2, 2023 19:33

mszabo-wikia force-pushed the migrate-to-vite branch from f2921bc to 426f021 Compare March 3, 2023 00:04

yurishkuro reviewed Mar 3, 2023

View reviewed changes

packages/jaeger-ui/index.html Show resolved Hide resolved

yurishkuro reviewed Mar 3, 2023

View reviewed changes

mszabo-wikia mentioned this pull request Mar 3, 2023

[Feature]: Split out page-specific libraries into dynamically imported chunks. #1233

Open

yurishkuro closed this Mar 7, 2023

yurishkuro reopened this Mar 7, 2023

yurishkuro previously approved these changes Mar 7, 2023

View reviewed changes

prettier

dfdb18f

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro dismissed their stale review via dfdb18f March 7, 2023 05:09

yurishkuro approved these changes Mar 7, 2023

View reviewed changes

yurishkuro merged commit 3a93b45 into jaegertracing:main Mar 7, 2023

yurishkuro reviewed Aug 14, 2023

View reviewed changes

anshgoyalevil mentioned this pull request Aug 15, 2023

Remove unused type defs and packages #1682

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate build tooling to Vite #1226

Migrate build tooling to Vite #1226

mszabo-wikia commented Mar 2, 2023

codecov bot commented Mar 2, 2023 •

edited

Loading

yurishkuro left a comment

yurishkuro Mar 2, 2023

yurishkuro Mar 2, 2023

mszabo-wikia Mar 2, 2023

yurishkuro Mar 2, 2023

mszabo-wikia Mar 2, 2023

mszabo-wikia Mar 2, 2023

yurishkuro Mar 2, 2023

mszabo-wikia Mar 2, 2023

mszabo-wikia Mar 2, 2023

mszabo-wikia Mar 3, 2023

yurishkuro Mar 2, 2023

mszabo-wikia Mar 2, 2023

yurishkuro Mar 2, 2023

mszabo-wikia Mar 2, 2023

yurishkuro Mar 2, 2023

yurishkuro Mar 2, 2023

yurishkuro left a comment

yurishkuro commented Mar 7, 2023

yurishkuro Aug 14, 2023

mszabo-wikia Aug 14, 2023

yurishkuro Aug 14, 2023

mszabo-wikia Aug 15, 2023

		@@ -1,59 +0,0 @@
		// Copyright (c) 2019 Uber Technologies, Inc.

		"@svgr/babel-plugin-transform-svg-component": "^6.5.1",
		"@svgr/babel-preset": "^6.5.1",

Migrate build tooling to Vite #1226

Migrate build tooling to Vite #1226

Conversation

mszabo-wikia commented Mar 2, 2023

Which problem is this PR solving?

Short description of the changes

Testing

codecov bot commented Mar 2, 2023 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 2, 2023 •

edited

Loading