-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Upgrade to React 18 #2527
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
Conversation
@testing-library/react includes new dependencies/internal changes that make use of import attributes. this forces either updating babel or using additional plugins
| Component: React.ComponentType<React.PropsWithoutRef<P>> | ||
| ): XComponentType<P> { | ||
| let forwardedRefComponent: XComponentType<P>; | ||
| function XComponent( | ||
| props: P, | ||
| props: React.PropsWithoutRef<P>, | ||
| ref: React.ForwardedRef<ComponentType<P>> | ||
| ): JSX.Element { | ||
| const ReplacementComponent = useXComponent(forwardedRefComponent); | ||
| const ReplacementComponent = useXComponent<P>(forwardedRefComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should test these types don't break DHE usages of XComponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2527 +/- ##
==========================================
- Coverage 44.95% 43.90% -1.05%
==========================================
Files 765 763 -2
Lines 42917 42867 -50
Branches 10796 10816 +20
==========================================
- Hits 19293 18821 -472
- Misses 23611 24031 +420
- Partials 13 15 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/components/src/spectrum/utils/useOnChangeTrackUncontrolled.test.ts
Show resolved
Hide resolved
| 'data-testid': dataTestId, | ||
| }: UISwitchProps): JSX.Element { | ||
| return ( | ||
| // eslint-disable-next-line jsx-a11y/control-has-associated-label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ignore is. The label is not (or I don't think we have a good label here for accessibility on the switch button). I have no idea why this rule is getting applied when it wasn't before. I'm guessing some minor bump when the package-lock had to be regenerated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I'd say just mark this UISwitch as deprecated, we should just use the Switch component from Spectrum instead anyway.
| Component: React.ComponentType<React.PropsWithoutRef<P>> | ||
| ): XComponentType<P> { | ||
| let forwardedRefComponent: XComponentType<P>; | ||
| function XComponent( | ||
| props: P, | ||
| props: React.PropsWithoutRef<P>, | ||
| ref: React.ForwardedRef<ComponentType<P>> | ||
| ): JSX.Element { | ||
| const ReplacementComponent = useXComponent(forwardedRefComponent); | ||
| const ReplacementComponent = useXComponent<P>(forwardedRefComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
| log.debug('closeFilePanel', fileMetadata); | ||
| const { id: fileId } = fileMetadata; | ||
| if (fileId == null) { | ||
| log.debug('File ID is null, ignore close event'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to make this a warn.
| log.debug('File ID is null, ignore close event'); | |
| log.warn('File ID is null, ignore close event'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya we can. Although debug matches the same error in a few other spots like just below this if the file is not open it just debug prints it's ignoring the close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually why are we even bothering? Change FileMetadata to require id to be a string - it is never null anywhere that I can see... or just throw an assertNotNull(fileId) here instead of quietly failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is null when creating a new notebook or sending text from the history to a new notebook. It looks like we set it to the file name in NotebookPanel#handleSaveSuccess
I don't want to make a change in this PR as it would be a behavior change
This reverts commit e4525ba.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question about the commented out code in table-operations.spec.ts.
Other than upgrading to React 18, also did the following
prop-typesalmost everywhere. There is 1 exportedprop-typeswe consume in DHE, so not removing fully yetdefaultPropson functional components. This is deprecated in 18 and removed completely in 19. We already had defaults with ES6 default params and having thecomponent.defaultPropsprints errors to the dev console that are annoyingreact-test-renderer. This is deprecated in 18 and removed in 19. Mainly had to updateGridandIrisGridtests, but it was relatively minimal changes, the tests are faster, and better because they test simulated clicks instead of emitting the change events directlyactor other issues like throwing without suppressing error logs that are expected. The only output now should be about deprecateddefaultPropson areact-beautiful-dndcomponent andfindDOMNodebeing deprecated for transitionsStyleGuide.test.tsx. We have e2e tests that are better and this unit test was causing issues withactbecause it has a bunch of lazy loaded components Jest can't resolve without changing the import mapIrisGridaround props and state that were incorrect