Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate build tooling to Vite #1226

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Migrate build tooling to Vite #1226

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
426f021
Migrate build tooling to Vite
mszabo-wikia Feb 26, 2023
dfdb18f
prettier
yurishkuro Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions BUILD.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,6 @@ In the project root, `typescript` is used to bolster the linting of TypeScript f

In `./packages/plexus`, `typescript` is used to generate type declarations for the ES module build. See [`./packages/plexus/BUILD.md`](packages/plexus/BUILD.md#typescript---emitdeclarationonly) for details.

### Workspaces

[Create React App](https://facebook.github.io/create-react-app/) (CRA) is used as the build-tooling for the Jaeger UI website. In 2.1.2+ CRA introduced a guard for the `start`, `build` and `test` scripts which checks the version of NPM packages available to make sure they're consistent with CRA's expectations ([reference](https://github.com/facebook/create-react-app/blob/dea19fdb30c2e896ed8ac75b68a612b0b92b2406/packages/react-scripts/scripts/utils/verifyPackageTree.js#L23-L29)). This process checks `node_modules` in parent directories and errors if an unexpected package version is encountered.

To avoid a world of pain, the [`nohoist`](https://yarnpkg.com/blog/2018/02/15/nohoist/#scope-private) feature of `yarn` workspaces is leveraged. CRA and it's dependencies are local to `./packages/jaeger-ui/node_modules` instead of `./node_modules`, i.e. they're not hoisted. This ensures CRA is using the packages it expects to use.

Unfortunately, the CRA check is not savvy to `yarn` workspaces and errors even though the _`yarn` workspace-magic_ ensures the right packages are actually used by the CRA scripts. So, the escape hatch provided by CRA is used to skip the check: the envvar `SKIP_PREFLIGHT_CHECK=true`, set in `./packages/jaeger-ui/.env`.

### Scripts

#### `build`
Expand Down
7 changes: 1 addition & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,7 @@
"packages/*"
],
"nohoist": [
"**/customize-cra",
"**/customize-cra/**",
"**/react-scripts",
"**/react-scripts/**",
"**/react-app-rewired",
"**/react-app-rewired/**"
"**/parse5"
]
},
"scripts": {
Expand Down
8 changes: 0 additions & 8 deletions packages/jaeger-ui/.env
View file
Open in desktop

This file was deleted.

11 changes: 11 additions & 0 deletions packages/jaeger-ui/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module.exports = {
// Let ESLint know about constants injected by the build system.
// These aren't technically globals (in that they are replaced by literals at build time),
// but from a linter perspective, they are globals and so need to be explicitly listed.
// https://vitejs.dev/config/shared-options.html#define
globals: {
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
__REACT_APP_GA_DEBUG__: false,
__REACT_APP_VSN_STATE__: false,
__APP_ENVIRONMENT__: false,
},
};
3 changes: 3 additions & 0 deletions packages/jaeger-ui/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
tsconfig.lint.tsbuildinfo

# Bundle size breakdown generated by rollup-plugin-visualizer
stats.html
24 changes: 0 additions & 24 deletions packages/jaeger-ui/config-overrides-antd-vars.less
View file
Open in desktop

This file was deleted.

73 changes: 0 additions & 73 deletions packages/jaeger-ui/config-overrides.js
View file
Open in desktop

This file was deleted.

0 packages/jaeger-ui/public/favicon.ico → packages/jaeger-ui/favicon.ico
View file
Open in desktop
File renamed without changes.
19 changes: 10 additions & 9 deletions packages/jaeger-ui/public/index.html → packages/jaeger-ui/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

<!-- NOTE: The document MUST have a <base> element. package.json#homepage is set to "." as part of resolving https://github.com/jaegertracing/jaeger-ui/issues/42 and therefore static assets are linked via relative URLs. This will break on many document URLs, e.g. /trace/abc, unless a valid base URL is provided. The base href defaults to "/" but the query-service can inject an override. -->
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
<base href="/" data-inject-target="BASE_URL" />
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<link rel="shortcut icon" href="/favicon.ico">
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
<title>Jaeger UI</title>
<script>
// Jaeger UI config data is embedded by the query-service via search-replace.
Expand All @@ -34,19 +34,20 @@
const JAEGER_VERSION = DEFAULT_VERSION;
return JAEGER_VERSION;
}

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

window.module = {};
</script>
</head>
<body>
<div id="jaeger-ui-root"></div>
<!--
This HTML file is a template.
If you open it directly in the browser, you will see an empty page.

You can add webfonts, meta tags, or analytics to this file.
The build step will place the bundled scripts into the <body> tag.
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved

To begin the development, run `npm start` in this folder.
To create a production bundle, use `npm run build`.
This file is the main entry point for the Jaeger UI application.
See https://vitejs.dev/guide/#index-html-and-project-root for more information
on how asset references are managed by the build system.
-->
<script type="module" src="/src/index.tsx"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screenshot 2023-03-02 at 20 27 38

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #1233 and #1234 to track these.

</body>
</html>
3 changes: 0 additions & 3 deletions packages/jaeger-ui/jest.global-setup.js
View file
Open in desktop

This file was deleted.

83 changes: 47 additions & 36 deletions packages/jaeger-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,44 +2,45 @@
"private": true,
"name": "jaeger-ui",
"version": "1.27.3",
"main": "src/index.js",
"main": "src/index.tsx",
"license": "Apache-2.0",
"homepage": ".",
"workspaces": {
"nohoist": [
"customize-cra",
"customize-cra/**",
"react-scripts",
"react-scripts/**",
"react-app-rewired",
"react-app-rewired/**"
]
},
"repository": {
"type": "git",
"url": "https://github.com/jaegertracing/jaeger-ui.git",
"directory": "packages/jaeger-ui"
},
"devDependencies": {
"@babel/core": "^7.21.0",
"@babel/preset-env": "^7.20.2",
"@babel/preset-react": "^7.18.6",
"@babel/preset-typescript": "^7.21.0",
"@svgr/babel-plugin-transform-svg-component": "^6.5.1",
"@svgr/babel-preset": "^6.5.1",
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, appreciate your comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reaching out! :)

"@types/match-sorter": "^2.3.0",
"@types/react-router-redux": "^5.0.21",
"@types/react-window": "^1.8.0",
"@types/redux-form": "^8.3.5",
"@types/rollup-plugin-visualizer": "^4.2.1",
"@vitejs/plugin-legacy": "^4.0.1",
"@vitejs/plugin-react": "^3.1.0",
"@wojtekmaj/enzyme-adapter-react-17": "^0.8.0",
"babel-plugin-import": "1.13.5",
"babel-jest": "^28.1.3",
"babel-plugin-inline-react-svg": "^2.0.2",
"bluebird": "^3.5.0",
"customize-cra": "0.2.9",
"enzyme": "^3.8.0",
"enzyme-to-json": "^3.6.2",
"http-proxy-middleware": "^2.0.6",
"jest": "^28.1.3",
"jest-environment-jsdom": "^28.1.3",
"jest-junit": "^15.0.0",
"less": "3.13.1",
"less-loader": "4.1.0",
"less-vars-to-js": "^1.2.1",
"react-app-rewired": "2.2.1",
"react-scripts": "2.1.3",
"react-test-renderer": "^18.2.0",
"rollup-plugin-visualizer": "^5.9.0",
"sinon": "7.3.2",
"source-map-explorer": "^2.5.3"
"source-map-explorer": "^2.5.3",
"terser": "^5.16.5",
"vite": "^4.1.4",
"vite-plugin-imp": "^2.3.1"
},
"dependencies": {
"@jaegertracing/plexus": "0.2.0",
Expand Down Expand Up @@ -104,7 +105,7 @@
"redux": "^3.7.2",
"redux-actions": "^2.2.1",
"redux-async-middleware": "^0.0.0",
"redux-form": "^8.3.8",
"redux-form": "^8.3.9",
"redux-promise-middleware": "^5.1.1",
"reselect": "^4.1.6",
"store": "^2.0.12",
Expand All @@ -115,17 +116,21 @@
"u-basscss": "2.0.1"
},
"scripts": {
"analyze": "source-map-explorer build/static/js/main.*",
"build": "REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) react-app-rewired build",
"coverage": "yarn run test --coverage",
"start:ga-debug": "REACT_APP_GA_DEBUG=1 REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) react-app-rewired start",
"start": "react-app-rewired start",
"test-dev": "react-app-rewired test --env=jsdom",
"test": "CI=1 react-app-rewired test --env=jsdom --color"
"build": "NODE_ENV=production REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) vite build",
"coverage": "yarn run test-ci --coverage",
"start:ga-debug": "REACT_APP_GA_DEBUG=1 REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) vite",
"start": "vite",
"test": "jest",
"test-ci": "CI=1 jest --color"
},
"jest": {
"globalSetup": "./jest.global-setup.js",
"globalSetup": "./test/jest.global-setup.js",
"setupFilesAfterEnv": [
"./test/jest-per-test-setup.js"
],
"collectCoverageFrom": [
"src/**/*.{js,jsx,ts,tsx}",
"!src/**/*.d.ts",
"!src/setup*.js",
"!src/utils/DraggableManager/demo/*.tsx",
"!src/utils/test/**/*.js",
Expand All @@ -135,12 +140,18 @@
"reporters": [
"default",
"jest-junit"
]
},
"browserslist": [
">0.5%",
"not dead",
"not ie <= 11",
"not op_mini all"
]
],
"transform": {
"\\.(css|png)$": "./test/generic-file-transform.js",
"\\.([jt]sx?|svg)$": "./test/babel-transform.js"
},
"transformIgnorePatterns": [
"[/\\\\\\\\]node_modules[/\\\\\\\\].+\\\\.(js|jsx|mjs|cjs|ts|tsx)$"
],
"testEnvironment": "jsdom",
"snapshotFormat": {
"escapeString": true,
"printBasicPrototype": true
}
}
}
4 changes: 2 additions & 2 deletions packages/jaeger-ui/src/components/common/FilteredList/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('<FilteredList>', () => {
describe('up / down arrow keys', () => {
let indices;

beforeAll(jest.useFakeTimers);
beforeAll(() => jest.useFakeTimers('modern'));

beforeEach(() => {
indices = {
Expand Down Expand Up @@ -266,7 +266,7 @@ describe('<FilteredList>', () => {
});

it('scrolling unsets the focus index', () => {
jest.useFakeTimers();
jest.useFakeTimers('modern');
wrapper.setState({ focusedIndex: 0 });
wrapper.instance().onListScrolled({ scrollUpdateWasRequested: false });
jest.runAllTimers();
Expand Down
Loading