refactor: Upgrade to React 17 #31961

kgabryje · 2025-01-22T19:31:41Z

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

villebro

🚀 ! A few quick comments

villebro · 2025-01-22T19:39:10Z

superset-frontend/plugins/legacy-plugin-chart-horizon/package.json

-    "react": "^15 || ^16"
+    "react": "^16 || ^17"


Here and elsewhere, could we just remove support for ^16? As we're doing breaking changes, let's not be shy with the sledgehammer! 😁

villebro · 2025-01-22T19:42:52Z

superset-frontend/spec/helpers/shim.tsx

@@ -23,7 +23,7 @@ import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only';
 import 'jest-enzyme';
 import jQuery from 'jquery';
 import { configure } from 'enzyme';
-import Adapter from 'enzyme-adapter-react-16';
+import Adapter from '@wojtekmaj/enzyme-adapter-react-17';


I see this is an "unofficial" package - am I reading this correctly that Enzyme doesn't support React > 16 natively?

Yep, that package is suggested as a substitute by chatgpt/stackoverflow. Hopefully though we can get rid of enzyme tests soon

villebro · 2025-01-22T19:47:52Z

superset-frontend/src/components/Datasource/CollectionTable.tsx

@@ -415,7 +415,7 @@ export default class CRUDCollection extends PureComponent<
      )),
    );
    if (allowAddItem) {
-      tds.push(<td key="add" />);
+      tds.push(<td key="add" aria-label="dd" />);


Was this supposed to be aria-label="Add"?

Oops, typo!

kgabryje · 2025-01-23T14:47:39Z

Status update:
While the project builds and runs just fine, there are multiple problems with the unit tests. For some reason Jest runtime environment triggers many errors that don't happen in regular runtime, such as problems with default exports (converting to a named export fixes the error, but I can't see a pattern - not all default errors are failing), problems with jest.spyOn (again - only some cases trigger an error, such as spying on superset-ui/core), errors like Cannot read properties of undefined (reading 'window') and many more. Overall over a 100 tests are failing right now.
I'm still trying to fix the jest and babel configs so that we don't need to manually adjust those tests. I've had limited luck so far (initially all tests were failing so I call it a success that at least some are passing now 😛) and GTP/Claude/Stackoverflow have not been very helpful. If I can't fix the tests with proper configuration, we'll need to do some code adjustments, not only in the tests, but the components too (for example converting problematic default exports to named ones).
CC @villebro @eschutho @mistercrunch @michael-s-molina

kgabryje · 2025-01-23T14:48:36Z

Of course, if you've got an idea on how to fix the configs, I'm all ears, any suggestion is more then welcome 🙂

kgabryje · 2025-01-23T15:20:39Z

superset-frontend/package.json

    "babel-jest": "^29.7.0",
    "babel-loader": "^9.1.3",
    "babel-plugin-dynamic-import-node": "^2.3.3",
    "babel-plugin-jsx-remove-data-test-id": "^3.0.0",
    "babel-plugin-lodash": "^3.3.4",
    "babel-plugin-typescript-to-proptypes": "^2.0.0",
+    "cheerio": "^1.0.0-rc.10",


enzyme tests break with the released version 1.0.0

villebro · 2025-01-23T17:17:04Z

Status update: While the project builds and runs just fine, there are multiple problems with the unit tests. For some reason Jest runtime environment triggers many errors that don't happen in regular runtime, such as problems with default exports (converting to a named export fixes the error, but I can't see a pattern - not all default errors are failing), problems with jest.spyOn (again - only some cases trigger an error, such as spying on superset-ui/core), errors like Cannot read properties of undefined (reading 'window') and many more. Overall over a 100 tests are failing right now. I'm still trying to fix the jest and babel configs so that we don't need to manually adjust those tests. I've had limited luck so far (initially all tests were failing so I call it a success that at least some are passing now 😛) and GTP/Claude/Stackoverflow have not been very helpful. If I can't fix the tests with proper configuration, we'll need to do some code adjustments, not only in the tests, but the components too (for example converting problematic default exports to named ones).

I feel we spend an unsustainable amount of time pampering old libraries and frameworks. Here's my two cents:

If Enzyme requires some unofficial library to even work with React 17, I feel we should just do away with all legacy Enzyme tests entirely. I don't see a lot of risk in this approach: old components that have Enzyme tests likely won't be touched that much anyways, and when they do, we should just be adding RTL tests to cover the changes. If we do hit a regression or two I think that's acceptable. At the end of the day, upgrading to React 17 is 100x more valuable than having some stale Enzyme tests lying around the codebase.
Unrelated, but in a similar way, I feel Cypress has become more of a burden than anything. Playwright has more or less completely replaced Cypress nowadays, and I feel we might be better off ripping that bandaid off, too, and even live without e2e tests, rather than spend all this time restarting flaky tests.

So I say: rip out all deps and tests that cause headaches with React 17.

rusackas · 2025-01-23T18:14:07Z

/korbit-review

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Invalid Regex Pattern in transformIgnorePatterns ▹ view

Files scanned

File Path	Reviewed
superset-frontend/src/components/ErrorMessage/IssueCode.tsx	✅
superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts	✅
superset-frontend/packages/superset-ui-core/src/chart/components/FallbackComponent.tsx	✅
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx	✅
superset-frontend/packages/superset-ui-core/src/chart-composition/legend/WithLegend.tsx	✅
superset-frontend/jest.config.js	✅
superset-frontend/babel.config.js	✅
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx	✅
superset-frontend/packages/superset-ui-core/src/chart/components/SuperChart.tsx	✅
superset-frontend/cypress-base/cypress/support/e2e.ts	✅
superset-frontend/src/components/Datasource/CollectionTable.tsx	✅
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

Setting Value

Review Schedule Automatic excluding drafts

Max Issue Count 10

Automatic PR Descriptions ❌

Issue Categories

Category Enabled

Naming ✅

Database Operations ✅

Documentation ✅

Logging ✅

Error Handling ✅

Systems and Environment ✅

Objects and Data Structures ✅

Readability and Maintainability ✅

Asynchronous Processing ✅

Design Patterns ✅

Third-Party Libraries ✅

Performance ✅

Security ✅

Functionality ✅

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

superset-frontend/jest.config.js

michael-s-molina · 2025-01-24T13:42:30Z

So I say: rip out all deps and tests that cause headaches with React 17.

I pulled this PR and executed the unit tests to collect some insights to help with this discussion:

We currently have 178/4545 tests failing (~4%)
Not all errors are related to Enzyme, we have many RTL tests failing. Some examples: DatasourceEditor.test.jsx, SavedQuerylist.test.jsx, DatasourceControl.test.tsx, AlertReportModal.test.tsx, FilterBart.test.tsx.

Given the low percentage of failing tests, IF we are able to resolve the problems with RTL tests, I would be OK with skipping failed Enzyme tests in favor of upgrading React.

mistercrunch · 2025-01-24T16:36:54Z

Any patterns identified in the failing, non-enzyme tests?

mistercrunch · 2025-01-24T16:38:14Z

Personally I support it.skip on enzyme test for the sake of moving forward, and take a note to either translate/rewrite/delete based on a case by case basis.

kgabryje · 2025-01-24T19:59:15Z

Update: we're in a pretty good place compared to yesterday! I skipped some failing enzyme tests and manually fixed most of the others.

kgabryje requested review from villebro, rusackas, sadpandajoe, geido, eschutho and betodealmeida as code owners January 22, 2025 19:31

pull-request-size bot added the size/L label Jan 22, 2025

github-actions bot added plugins dependencies:npm packages labels Jan 22, 2025

dosubot bot added change:frontend Requires changing the frontend risk:breaking-change Issues or PRs that will introduce breaking changes labels Jan 22, 2025

kgabryje marked this pull request as draft January 22, 2025 19:32

villebro reviewed Jan 22, 2025

View reviewed changes

kgabryje force-pushed the chore/react-17-upgrade branch from 8c87baf to fc875b4 Compare January 23, 2025 09:17

kgabryje marked this pull request as ready for review January 23, 2025 14:31

michael-s-molina changed the title ~~chore: Upgrade to React 17~~ refactor: Upgrade to React 17 Jan 23, 2025

kgabryje commented Jan 23, 2025

View reviewed changes

korbit-ai bot reviewed Jan 23, 2025

View reviewed changes

superset-frontend/jest.config.js Outdated Show resolved Hide resolved

kgabryje added 4 commits January 24, 2025 20:34

Upgrade to React 17

87f55be

Fix type

eba6fc5

fix conflict

1f97894

fix tests config

9d4f81a

Fix some tests

920b256

kgabryje force-pushed the chore/react-17-upgrade branch from a11dbec to 920b256 Compare January 24, 2025 19:36

pull-request-size bot added size/XL and removed size/L labels Jan 24, 2025

add missing dep

ec6c2a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Upgrade to React 17 #31961

refactor: Upgrade to React 17 #31961

kgabryje commented Jan 22, 2025

villebro left a comment

villebro Jan 22, 2025

kgabryje Jan 22, 2025

villebro Jan 22, 2025

kgabryje Jan 22, 2025

villebro Jan 22, 2025

kgabryje Jan 22, 2025

kgabryje commented Jan 23, 2025

kgabryje commented Jan 23, 2025

kgabryje Jan 23, 2025

rusackas Jan 23, 2025

villebro commented Jan 23, 2025

rusackas commented Jan 23, 2025

korbit-ai bot left a comment •

edited

Loading

michael-s-molina commented Jan 24, 2025

mistercrunch commented Jan 24, 2025

mistercrunch commented Jan 24, 2025

kgabryje commented Jan 24, 2025

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

refactor: Upgrade to React 17 #31961

Are you sure you want to change the base?

refactor: Upgrade to React 17 #31961

Conversation

kgabryje commented Jan 22, 2025

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro 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

kgabryje commented Jan 23, 2025

kgabryje commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro commented Jan 23, 2025

rusackas commented Jan 23, 2025

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Current Korbit Configuration

Feedback and Support

Looking to add Korbit to your team? Get started with a free 2 week trial here

michael-s-molina commented Jan 24, 2025

mistercrunch commented Jan 24, 2025

mistercrunch commented Jan 24, 2025

kgabryje commented Jan 24, 2025

korbit-ai bot left a comment •

edited

Loading