Migrate annotations to react18 #716
Conversation
Migrated codebase from React 17 to React 18. Updated `ReactDOM` imports and usage across multiple files to use `ReactDOM.client`. This included changes to various test files, components, utilities, and the package dependencies.
Simplified import syntax across multiple components, moving from wildcard imports to direct imports. Removed unnecessary `usePrevious` hooks to streamline popup update logic and updated `yarn.lock` to newer versions of several dependencies.
Refactored components to improve readability and consistency. Updated package dependencies and ESLint configurations for better code quality and compatibility with the latest libraries. Adjustments include moving to "react-jsx," improving TypeScript definitions, and simplifying utility functions.
Mocking the entire react-redux module to control useSelector behavior. This ensures test consistency by avoiding real state dependencies.
Replaced `ReactRedux.useSelector` with `useSelector` in PopupReply for consistency. Also, removed unused and commented-out code in global type definitions to improve code clarity.
Added a directive to disable ESLint checks for undefined variables in the popperMock.js script. This change ensures that the createPopper mock function can be defined without ESLint errors during Jest testing.
| // const removeChildSpy = jest.spyOn(rootEl, 'removeChild'); | ||
| const wrapper = getWrapper(); | ||
| const root = ReactDOM.createRoot(rootEl); | ||
| wrapper.destroy(); | ||
|
|
||
| expect(ReactDOM.unmountComponentAtNode).toHaveBeenCalledWith(wrapper.reactEl); | ||
| expect(removeChildSpy).toHaveBeenCalledWith(wrapper.reactEl); | ||
| expect(root.unmount).toHaveBeenCalled(); | ||
| // expect(removeChildSpy).toHaveBeenCalledWith(wrapper.reactEl); |
There was a problem hiding this comment.
Looks like these comments may have got left here on accident. Also, is there an equivalent to removing the node that should be tested?
| if (popup?.popper && (rotation !== prevRotation || scale !== prevScale)) { | ||
| if (popup?.popper) { | ||
| popup.popper.update(); | ||
| } |
There was a problem hiding this comment.
Will there be any downside to this updating every time? If not, we can also look at removing usePrevious hook since this file looks like only reference.
There was a problem hiding this comment.
What I found through testing is that with the new updates in React18 this was no longer getting updated as it was before making the usePrevious hook obsolete.
src/drawing/DrawingCreator.tsx
Outdated
| @@ -1,4 +1,4 @@ | |||
| import React from 'react'; | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
nit - I saw other files were changed to use import React from 'react';, could that be used here too for consistency?
| // test('should set the root and annotated element based on class name', () => { | ||
| // annotator.init(2); | ||
| // | ||
| // expect(annotator.containerEl).toBe(container); | ||
| // expect(annotator.annotatedEl).toBe(getParent()); | ||
| // expect(annotator.render).toHaveBeenCalled(); | ||
| // }); |
There was a problem hiding this comment.
There are a couple of commented out tests in this file. Are these able to be migrated easily?
src/store/users/actions.ts
Outdated
| filter_term: searchString, | ||
|
|
||
| include_groups: false, | ||
|
|
||
| include_uploader_collabs: false, |
There was a problem hiding this comment.
nit - is new line between each intentional?
* In BaseManager, added back deleting the root react node on destroy * Removed usePrevious files - no longer used * Added mocks of a mangager to fix ImageAnnotator unit tests of getReference and init functions * Minor style changes
src/drawing/DrawingCreator.tsx
Outdated
| @@ -35,6 +35,7 @@ export default function DrawingCreator({ | |||
| const drawingPathRef = React.useRef<DrawingPathRef>(null); | |||
| const drawingSVGRef = React.useRef<DrawingSVGRef>(null); | |||
| const renderHandleRef = React.useRef<number | null>(null); | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
I was able to change it to use useMemo instead.
| "@types/react": "^17.0.2", | ||
| "@types/react-dom": "^17.0.1", | ||
| "@types/react-redux": "^7.1.16", | ||
| "@types/react": "18.3.12", |
There was a problem hiding this comment.
Why is there a type mismatch between @types/react and react?
There was a problem hiding this comment.
I found this thread about it. The DefinitelyTyped library uses major and minor versions that match the corresponding library but varies on patch version.
This pull request includes several updates and improvements to the codebase, primarily focusing on upgrading dependencies, updating ESLint configurations, and refactoring React-related code to support React 18. The most important changes include updating the ESLint configuration, Typescript upgrade, upgrading various dependencies in
package.json, and refactoring ReactDOM usage to align with React 18.ESLint Configuration Updates:
.eslintrc.js: Updated theextendsproperty to include separate configurations for base, react, and typescript. Also, replaced deprecated@typescript-eslint/camelcaserule with@typescript-eslint/naming-conventionand addedimport/no-import-module-exportsrule. [1] [2]Dependency Upgrades:
package.json: Upgraded several Babel, ESLint, and React dependencies to their latest versions. Notable changes include upgrading@babel/*packages,eslintand related plugins, and moving from React 17 to React 18. [1] [2] [3] [4] [5] [6]ReactDOM Refactoring:
src/common/BaseManager.ts: Updated to useReactDOM.createRootandroot.unmountfor React 18 compatibility. [1] [2] [3]src/common/DeselectManager.tsx: Modified to useReactDOM.createRootandroot.renderfor React 18 compatibility. [1] [2]src/common/__tests__/BaseManager-test.ts: Adjusted tests to mockReactDOM.createRootandroot.unmountfor React 18. [1] [2]src/common/__tests__/DeselectManager-test.tsx: Updated tests to mockReactDOM.createRootandroot.renderfor React 18. [1] [2]Utility and Component Updates:
src/components/Popups/Popper.ts: AddedclientBoundingRectutility function for consistent DOMRect creation.PopupCursor.tsx,PopupHighlight.tsx,PopupHighlightError.tsx,PopupReply.tsx): Updated to useclientBoundingRectfor creating bounding client rectangles. [1] [2] [3] [4] [5] [6] [7]