Skip to content

Commit

Permalink
Migrate build tooling to Vite (jaegertracing#1226)
Browse files Browse the repository at this point in the history
## 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ó <mszabo@fandom.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
  • Loading branch information
2 people authored and Benjamin Klein committed Apr 18, 2023
1 parent 76c39d0 commit 6fa1cf0
Show file tree
Hide file tree
Showing 25 changed files with 1,783 additions and 6,106 deletions.
8 changes: 0 additions & 8 deletions BUILD.md
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
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

This file was deleted.

11 changes: 11 additions & 0 deletions packages/jaeger-ui/.eslintrc.js
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: {
__REACT_APP_GA_DEBUG__: false,
__REACT_APP_VSN_STATE__: false,
__APP_ENVIRONMENT__: false,
},
};
3 changes: 3 additions & 0 deletions packages/jaeger-ui/.gitignore
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

This file was deleted.

73 changes: 0 additions & 73 deletions packages/jaeger-ui/config-overrides.js

This file was deleted.

File renamed without changes.
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. -->
<base href="/" data-inject-target="BASE_URL" />
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<link rel="shortcut icon" href="/favicon.ico">
<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.
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.
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>
</body>
</html>
3 changes: 0 additions & 3 deletions packages/jaeger-ui/jest.global-setup.js

This file was deleted.

83 changes: 47 additions & 36 deletions packages/jaeger-ui/package.json
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",
"@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
}
}
}
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

0 comments on commit 6fa1cf0

Please sign in to comment.