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.

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

chore: Upgrade to React 17 #31961

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kgabryje
Copy link
Member

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

Copy link

korbit-ai bot commented Jan 22, 2025

Korbit doesn't automatically review large (500+ lines changed) pull requests such as this one. If you want me to review anyway, use /korbit-review.

@dosubot 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 kgabryje marked this pull request as draft January 22, 2025 19:32
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

🚀 ! A few quick comments

"react": "^15 || ^16"
"react": "^16 || ^17"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

🛠️

@@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -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" />);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, typo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend dependencies:npm packages plugins risk:breaking-change Issues or PRs that will introduce breaking changes size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants