Help needed with upgrading dependencies #1199
Labels
Comments
|
I think this would be good for reference: For the following PR: I think the problem is in the
ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it’s running React 17. Please refer to this blog for more details If possible, please explain how we can help in cleaning this up and keeping the Jaeger UI up to date? Thank You! |
|
@SaarthakMaini thanks for offering to help. I suspect your analysis above may apply to some upgrades, but others might require other changes. Either way, the way to proceed is to tackle dependencies one by one. The link in the description points to many unresolved PRs by dependabot, which are failing CI. You can take any of them, run locally to see what |
This was referenced Feb 25, 2023
yurishkuro
added a commit
that referenced
this issue
Mar 7, 2023
## 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]>
Merged
yurishkuro
pushed a commit
that referenced
this issue
Mar 10, 2023
## Which problem is this PR solving? - Contributes towards #1199 ## Short description of the changes With the TS update it now seems possible to update `antd`. There are two eventual benefits: * antd v4 contains React 18-compatible typings, * antd v4 supports [treeshakeable SVG icons](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md#icon-upgrade), reducing the bundle size. The [migration guide](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md) recommends updating to the latest v3 release before going to v4. Apart from ubiquitous snapshot changes, a few adjustments were necessary in components and tests: * The test case for the `<Page />` component that aimed to verify that the header was not rendered in embedded mode was only working by accident since 53937eb—the `Header` component didn't have a display name, so the test wouldn't find it even though the test setup was no longer correct. Fix the test setup to setup the `embedded` prop accordingly. * antd's `Menu` component is now wrapped in a `Context.Consumer`, so tests using enzyme's shallow rendering need to `dive` into it to be able to find the child components they are looking for. * The `Upload` component has more strictly typed props now, so the signature of the `loadJsonFile` prop had to be adjusted accordingly. The handling code was only accessing the `file` property of `FileList`, and the real type supplied by the event handler also has a `file` property, so no code changes were required apart from the typing update. * The `Table` component has one more internal wrapper (each passing down the `className` prop to the next), so the test in `TraceSpanView` had to be updated accordingly. * The `onCell` event handler for `Table`s is now strictly typed, so `DetailTable` needed an update to return an empty object `{}` if it intends to set no props. This is consistent with the upstream [examples](https://github.com/react-component/table/blob/83cce50ddb0a55d63bb0f435d3f4ff7e3301ebad/docs/examples/colspan-rowspan.tsx#L25). * `KeyboardShortcutsHelp` was setting a spurious `align` prop on `Modal`, which was already not part of its typings in 3.9.0 but was still present as a prop type. This caused a type error now that the prop type was removed. * In manual testing, I observed that the name selectors (e.g. "Group By") on the trace statistics view were not functioning—they would immediately close when clicking to open them. I thought this was a regression caused by the update, but experienced the same behavior on `main`. This was fixed by deferring event handler registration to ensure the event handlers don't end up catching the same event that triggered the original state change. ## Screenshots     --------- Signed-off-by: Máté Szabó <[email protected]>
yurishkuro
added a commit
that referenced
this issue
Mar 10, 2023
## Which problem is this PR solving? - Contributes towards #1199 ## Short description of the changes Bump ESLint to v7 and update related plugins. Suppress newly introduced or updated rules in a separate config section so that they can be evaluated and enabled later, except for stylistic rules that do not match the existing code style, or where the impact was limited enough to be trivially fixable or suppressable. `@typescript-eslint/eslint-plugin` now validates that input files are indeed part of the tsconfig file configured in ESLint options. Since the plugin doesn't understand project references, this requires a small change to [specify the package-specific tsconfig files for linting](https://typescript-eslint.io/linting/typed-linting/monorepos#one-tsconfigjson-per-package-and-an-optional-one-in-the-root) instead. Also remove `eslint-config-react-app` (the CRA ESLint config), as it doesn't actually seem to bring much to the table—the Airbnb config seems to overwrite much of the same rules that this config is trying to provide. I ran `eslint --print-config packages/jaeger-ui/src/components/App/index.jsx` and `eslint --print-config packages/jaeger-ui/src/components/App/Page.tsx` to dump the active rules for JS and TS files with and without the config loaded, then diffed them: ```diff --- ts-active-rules-cra.json 2023-03-10 21:24:37 +++ ts-active-rules-no-cra.json 2023-03-10 21:25:38 @@ -4,8 +4,7 @@ "jest": true, "jasmine": true, "es6": true, - "node": true, - "commonjs": true + "node": true }, "globals": { "__REACT_APP_GA_DEBUG__": false, @@ -27,7 +26,6 @@ "sourceType": "module" }, "plugins": [ - "flowtype", "import", "react", "jsx-a11y", @@ -2850,15 +2848,6 @@ ], "yoda": [ "error" - ], - "flowtype/define-flow-type": [ - "warn" - ], - "flowtype/require-valid-file-annotation": [ - "warn" - ], - "flowtype/use-flow-type": [ - "warn" ] }, "settings": { ``` ```diff --- js-active-rules-cra.json 2023-03-10 21:24:48 +++ js-active-rules-no-cra.json 2023-03-10 21:25:30 @@ -4,15 +4,14 @@ "jest": true, "jasmine": true, "es6": true, - "node": true, - "commonjs": true + "node": true }, "globals": { "__REACT_APP_GA_DEBUG__": false, "__REACT_APP_VSN_STATE__": false, "__APP_ENVIRONMENT__": false }, - "parser": "/REDACTED/jaeger-ui/node_modules/babel-eslint/lib/index.js", + "parser": null, "parserOptions": { "ecmaFeatures": { "jsx": true, @@ -23,7 +22,6 @@ "sourceType": "module" }, "plugins": [ - "flowtype", "import", "react", "jsx-a11y" @@ -2830,15 +2828,6 @@ ], "yoda": [ "error" - ], - "flowtype/define-flow-type": [ - "warn" - ], - "flowtype/require-valid-file-annotation": [ - "warn" - ], - "flowtype/use-flow-type": [ - "warn" ] }, "settings": { ``` So the only things added by the config is Flow support (which the project doesn't use), and extended ES6 support via `babel-eslint` (which is abandoned in favor of `@babel/eslint-parser`). As such, remove `eslint-plugin-flowtype` as well and replace `babel-eslint` with a simple `@babel/eslint-parser` config. --------- Signed-off-by: Máté Szabó <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
|
@mszabo-wikia do you have a sense of the remaining complexity for these upgrade? You've been doing such a great job pushing upgrades, I'd like to see you be able to call it a victory soon. |
|
@yurishkuro Thank you :) I think there are a few different things left:
Hope this helps! Let me know if you have any questions. |
Binrix
pushed a commit
to Binrix/jaeger-ui
that referenced
this issue
Apr 18, 2023
## 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]>
Binrix
pushed a commit
to Binrix/jaeger-ui
that referenced
this issue
Apr 18, 2023
## Which problem is this PR solving? - Contributes towards jaegertracing#1199 ## Short description of the changes With the TS update it now seems possible to update `antd`. There are two eventual benefits: * antd v4 contains React 18-compatible typings, * antd v4 supports [treeshakeable SVG icons](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md#icon-upgrade), reducing the bundle size. The [migration guide](https://github.com/ant-design/ant-design/blob/4.24.8/docs/react/migration-v4.en-US.md) recommends updating to the latest v3 release before going to v4. Apart from ubiquitous snapshot changes, a few adjustments were necessary in components and tests: * The test case for the `<Page />` component that aimed to verify that the header was not rendered in embedded mode was only working by accident since 53937eb—the `Header` component didn't have a display name, so the test wouldn't find it even though the test setup was no longer correct. Fix the test setup to setup the `embedded` prop accordingly. * antd's `Menu` component is now wrapped in a `Context.Consumer`, so tests using enzyme's shallow rendering need to `dive` into it to be able to find the child components they are looking for. * The `Upload` component has more strictly typed props now, so the signature of the `loadJsonFile` prop had to be adjusted accordingly. The handling code was only accessing the `file` property of `FileList`, and the real type supplied by the event handler also has a `file` property, so no code changes were required apart from the typing update. * The `Table` component has one more internal wrapper (each passing down the `className` prop to the next), so the test in `TraceSpanView` had to be updated accordingly. * The `onCell` event handler for `Table`s is now strictly typed, so `DetailTable` needed an update to return an empty object `{}` if it intends to set no props. This is consistent with the upstream [examples](https://github.com/react-component/table/blob/83cce50ddb0a55d63bb0f435d3f4ff7e3301ebad/docs/examples/colspan-rowspan.tsx#L25). * `KeyboardShortcutsHelp` was setting a spurious `align` prop on `Modal`, which was already not part of its typings in 3.9.0 but was still present as a prop type. This caused a type error now that the prop type was removed. * In manual testing, I observed that the name selectors (e.g. "Group By") on the trace statistics view were not functioning—they would immediately close when clicking to open them. I thought this was a regression caused by the update, but experienced the same behavior on `main`. This was fixed by deferring event handler registration to ensure the event handlers don't end up catching the same event that triggered the original state change. ## Screenshots     --------- Signed-off-by: Máté Szabó <[email protected]>
Binrix
pushed a commit
to Binrix/jaeger-ui
that referenced
this issue
Apr 18, 2023
## Which problem is this PR solving? - Contributes towards jaegertracing#1199 ## Short description of the changes Bump ESLint to v7 and update related plugins. Suppress newly introduced or updated rules in a separate config section so that they can be evaluated and enabled later, except for stylistic rules that do not match the existing code style, or where the impact was limited enough to be trivially fixable or suppressable. `@typescript-eslint/eslint-plugin` now validates that input files are indeed part of the tsconfig file configured in ESLint options. Since the plugin doesn't understand project references, this requires a small change to [specify the package-specific tsconfig files for linting](https://typescript-eslint.io/linting/typed-linting/monorepos#one-tsconfigjson-per-package-and-an-optional-one-in-the-root) instead. Also remove `eslint-config-react-app` (the CRA ESLint config), as it doesn't actually seem to bring much to the table—the Airbnb config seems to overwrite much of the same rules that this config is trying to provide. I ran `eslint --print-config packages/jaeger-ui/src/components/App/index.jsx` and `eslint --print-config packages/jaeger-ui/src/components/App/Page.tsx` to dump the active rules for JS and TS files with and without the config loaded, then diffed them: ```diff --- ts-active-rules-cra.json 2023-03-10 21:24:37 +++ ts-active-rules-no-cra.json 2023-03-10 21:25:38 @@ -4,8 +4,7 @@ "jest": true, "jasmine": true, "es6": true, - "node": true, - "commonjs": true + "node": true }, "globals": { "__REACT_APP_GA_DEBUG__": false, @@ -27,7 +26,6 @@ "sourceType": "module" }, "plugins": [ - "flowtype", "import", "react", "jsx-a11y", @@ -2850,15 +2848,6 @@ ], "yoda": [ "error" - ], - "flowtype/define-flow-type": [ - "warn" - ], - "flowtype/require-valid-file-annotation": [ - "warn" - ], - "flowtype/use-flow-type": [ - "warn" ] }, "settings": { ``` ```diff --- js-active-rules-cra.json 2023-03-10 21:24:48 +++ js-active-rules-no-cra.json 2023-03-10 21:25:30 @@ -4,15 +4,14 @@ "jest": true, "jasmine": true, "es6": true, - "node": true, - "commonjs": true + "node": true }, "globals": { "__REACT_APP_GA_DEBUG__": false, "__REACT_APP_VSN_STATE__": false, "__APP_ENVIRONMENT__": false }, - "parser": "/REDACTED/jaeger-ui/node_modules/babel-eslint/lib/index.js", + "parser": null, "parserOptions": { "ecmaFeatures": { "jsx": true, @@ -23,7 +22,6 @@ "sourceType": "module" }, "plugins": [ - "flowtype", "import", "react", "jsx-a11y" @@ -2830,15 +2828,6 @@ ], "yoda": [ "error" - ], - "flowtype/define-flow-type": [ - "warn" - ], - "flowtype/require-valid-file-annotation": [ - "warn" - ], - "flowtype/use-flow-type": [ - "warn" ] }, "settings": { ``` So the only things added by the config is Flow support (which the project doesn't use), and extended ES6 support via `babel-eslint` (which is abandoned in favor of `@babel/eslint-parser`). As such, remove `eslint-plugin-flowtype` as well and replace `babel-eslint` with a simple `@babel/eslint-parser` config. --------- Signed-off-by: Máté Szabó <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We have many dependabot PRs that are failing CI for various reasons, like mismatching type declarations or API changes in the dependency. Looking for community help to clean this up and keep Jaeger UI up to date.
All outstanding dependency upgrades by label.
The text was updated successfully, but these errors were encountered: