Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

fix: LEAP-25: Eliminate memory leaks introduces by React 17 by upgrading to React 18 #1538

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions e2e/tests/regression-tests/annotation-button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ Scenario('Annotation button should keep border width on hover', async ({ I, Labe
],
});

const borderWidth = await I.executeScript(()=>{
LabelStudio.waitForObjectsReady();
I.seeElement('.lsf-annotation-button:nth-child(2)');
const borderWidth = await I.executeScript(() => {
const el = document.querySelector('.lsf-annotation-button:nth-child(2)');
const borderWidth = window.getComputedStyle(el).getPropertyValue('border-width');

Expand All @@ -36,7 +38,7 @@ Scenario('Annotation button should keep border width on hover', async ({ I, Labe

I.moveCursorTo('.lsf-annotation-button:nth-child(2)');

const borderWidthHovered = await I.executeScript(()=>{
const borderWidthHovered = await I.executeScript(() => {
const el = document.querySelector('.lsf-annotation-button:nth-child(2)');
const borderWidth = window.getComputedStyle(el).getPropertyValue('border-width');

Expand All @@ -48,4 +50,4 @@ Scenario('Annotation button should keep border width on hover', async ({ I, Labe
borderWidthHovered,
'Annotation button\'s border width should not change on hover',
);
});
});
19 changes: 11 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,12 @@
"@babel/preset-react": "7.18.6",
"@babel/preset-typescript": "7.18.6",
"@babel/runtime": "7.18.6",
"@cfaester/enzyme-adapter-react-18": "0.7.0",
"@heartexlabs/eslint-plugin-frontend": "https://github.com/heartexlabs/eslint-plugin-frontend.git",
"@svgr/webpack": "^8.0.1",
"@testing-library/react": "12.1.2",
"@testing-library/jest-dom": "6.1.1",
"@testing-library/react": "14.0.0",
"@testing-library/user-event": "14.4.3",
"@types/chroma-js": "^2.1.3",
"@types/enzyme": "^3.10.12",
"@types/jest": "^29.2.3",
Expand All @@ -125,11 +128,11 @@
"@types/mini-css-extract-plugin": "^2.5.1",
"@types/nanoid": "^3.0.0",
"@types/offscreencanvas": "^2019.6.4",
"@types/react-dom": "^17.0.11",
"@types/react": "18.2.21",
"@types/react-dom": "18.2.7",
"@types/react-window": "^1.8.5",
"@types/strman": "^2.0.0",
"@types/wavesurfer.js": "^6.0.0",
"@wojtekmaj/enzyme-adapter-react-17": "^0.4.1",
"antd": "^4.3.3",
"autoprefixer": "^10.4.2",
"babel-jest": "^29.3.1",
Expand Down Expand Up @@ -164,9 +167,9 @@
"lodash.ismatch": "^4.4.0",
"lodash.throttle": "^4.1.1",
"mini-css-extract-plugin": "^2.7.5",
"mobx": "^5.15.4",
"mobx-react": "^6",
"mobx-state-tree": "^3.16.0",
"mobx": "6.10.2",
"mobx-react": "7",
"mobx-state-tree": "5.2.0",
"nanoid": "^3.3.0",
"node-fetch": "^2.6.1",
"pleasejs": "^0.4.2",
Expand All @@ -175,8 +178,8 @@
"postcss-preset-env": "^7.4.1",
"prettier": "^1.19.1",
"raw-loader": "^4.0.2",
"react": "^17.0.2",
"react-dom": "^17.0.2",
"react": "18.2.0",
"react-dom": "18.2.0",
"react-hot-loader": "^4.13.0",
"react-konva": "^17.0.2-0",
"react-rating": "^1.6.2",
Expand Down
56 changes: 39 additions & 17 deletions src/LabelStudio.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { render, unmountComponentAtNode } from 'react-dom';
import { createRoot } from 'react-dom/client';
import App from './components/App/App';
import { configureStore } from './configureStore';
import { LabelStudio as LabelStudioReact } from './Component';
Expand All @@ -14,6 +14,7 @@ import { destroy } from 'mobx-state-tree';
import { destroy as destroySharedStore } from './mixins/SharedChoiceStore/mixin';
import { cleanDomAfterReact, findReactKey } from './utils/reactCleaner';
import { FF_LSDV_4620_3_ML, isFF } from './utils/feature-flags';
import { StrictMode } from 'react';

configure({
isolateGlobalState: true,
Expand Down Expand Up @@ -60,6 +61,7 @@ export class LabelStudio {
async createApp() {
const { store, getRoot } = await configureStore(this.options, this.events);
const rootElement = getRoot(this.root);
const appRoot = createRoot(rootElement);

this.store = store;
window.Htx = this.store;
Expand All @@ -68,30 +70,50 @@ export class LabelStudio {

const renderApp = () => {
if (isRendered) {
clearRenderedApp();
unmountApp();
}
render((
<App
store={this.store}
panels={registerPanels(this.options.panels) ?? []}
/>
), rootElement);
appRoot.render(
<StrictMode>
<App
store={this.store}
panels={registerPanels(this.options.panels) ?? []}
/>
</StrictMode>,
);
};

const clearRenderedApp = () => {
const childNodes = [...rootElement.childNodes];
// cleanDomAfterReact needs this key to be sure that cleaning affects only current react subtree
const reactKey = findReactKey(childNodes[0]);
const PERFORM_REACT_CLEANUP = true;

const cleanupReactNodes = (callback) => {
let childNodes, reactKey;

if (PERFORM_REACT_CLEANUP) {
childNodes = Array.from(rootElement.childNodes);
console.log(childNodes);

// cleanDomAfterReact needs this key to be sure that cleaning affects
// only current react subtree
reactKey = findReactKey(childNodes[0]);
}

callback();

unmountComponentAtNode(rootElement);
/*
Unmounting doesn't help with clearing React's fibers
but removing the manually helps
@see https://github.com/facebook/react/pull/20290 (similar problem)
That's maybe not relevant in version 18
*/
cleanDomAfterReact(childNodes, reactKey);
cleanDomAfterReact([rootElement], reactKey);
if (childNodes && reactKey) {
cleanDomAfterReact(childNodes, reactKey);
cleanDomAfterReact([rootElement], reactKey);
}
};

const unmountApp = () => {
cleanupReactNodes(() => {
appRoot.unmount();
});
};

renderApp();
Expand All @@ -100,12 +122,12 @@ export class LabelStudio {
return isRendered;
},
render: renderApp,
clear: clearRenderedApp,
clear: unmountApp,
});

this.destroy = () => {
if (isFF(FF_LSDV_4620_3_ML)) {
clearRenderedApp();
unmountApp();
}
destroySharedStore();
if (isFF(FF_LSDV_4620_3_ML)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
/* global test, expect, jest, describe */
import Enzyme, { mount } from 'enzyme';
import Adapter from '@wojtekmaj/enzyme-adapter-react-17';
import { render } from '@testing-library/react';
import { AnnotationButton } from '../AnnotationButton';
// eslint-disable-next-line
// @ts-ignore
import { annotationStore } from './sampleData.js';

Enzyme.configure({ adapter: new Adapter() });
import '@testing-library/jest-dom';

jest.mock('react', () => ({
...jest.requireActual('react'),
Expand All @@ -16,14 +14,15 @@ jest.mock('react', () => ({
describe('AnnotationsButton', () => {
test('Annotation', () => {
const entity = annotationStore.annotations[0];
const view = mount(<AnnotationButton entity={entity} capabilities={{}} annotationStore={annotationStore} />);
const view = render(<AnnotationButton entity={entity} capabilities={{}} annotationStore={annotationStore} />);

expect(view.find('.dm-annotation-button__entity-id').text()).toBe(`#${entity.pk}`);
expect(view.getByText(`#${entity.pk}`)).toBeInTheDocument();
});

test('Prediction', () => {
const entity = annotationStore.predictions[0];
const view = mount(<AnnotationButton entity={entity} capabilities={{}} annotationStore={annotationStore} />);
expect(view.find('.dm-annotation-button__entity-id').text()).toBe(`#${entity.pk}`);
const view = render(<AnnotationButton entity={entity} capabilities={{}} annotationStore={annotationStore} />);

expect(view.getByText(`#${entity.pk}`)).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
/* global test, expect, jest */
import React from 'react';
import Enzyme, { mount } from 'enzyme';
import Adapter from '@wojtekmaj/enzyme-adapter-react-17';
import { render } from '@testing-library/react';
import { AnnotationsCarousel } from '../AnnotationsCarousel';
// eslint-disable-next-line
// @ts-ignore
import { annotationStore, store } from './sampleData.js';

Enzyme.configure({ adapter: new Adapter() });

jest.mock('react', () => ({
...jest.requireActual('react'),
useLayoutEffect: jest.requireActual('react').useEffect,
}));

test('AnnotationsCarousel', async () => {
const view = mount(<AnnotationsCarousel annotationStore={annotationStore} store={store} />);

expect(view.find('.dm-annotations-carousel__carosel').children().length).toBe(9);
const view = render(
<AnnotationsCarousel
annotationStore={annotationStore}
store={store}
/>,
);
const carouselItems = view.container.querySelectorAll('.dm-annotations-carousel__carosel > *');

expect(carouselItems).toHaveLength(9);
});
11 changes: 3 additions & 8 deletions src/components/Hint/Hint.test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
/* global describe, it, expect */
import React from 'react';
import Enzyme, { shallow } from 'enzyme';
import { shallowToJson } from 'enzyme-to-json';
import Adapter from '@wojtekmaj/enzyme-adapter-react-17';

Enzyme.configure({ adapter: new Adapter() });

import Hint from './Hint';
import { render } from '@testing-library/react';

describe('Hint', () => {
it('Should render correctly', () => {
Expand All @@ -16,8 +11,8 @@ describe('Hint', () => {
</Hint>
);

const output = shallow(component);
const output = render(component);

expect(shallowToJson(output)).toMatchSnapshot();
expect(output.asFragment()).toMatchSnapshot();
});
});
12 changes: 7 additions & 5 deletions src/components/SidePanels/PanelBase.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { FC, MutableRefObject, MouseEvent as RMouseEvent, useCallback, useEffect, useMemo, useRef, useState } from 'react';
import React, { FC, MutableRefObject, MouseEvent as RMouseEvent, useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { Block, Elem } from '../../utils/bem';
import { IconArrowLeft, IconArrowRight, IconOutlinerCollapse, IconOutlinerExpand } from '../../assets/icons';

Expand Down Expand Up @@ -32,7 +32,7 @@ const resizers = [
interface PanelBaseProps {
root: MutableRefObject<HTMLDivElement | undefined>;
name: PanelType;
mix?: string|string[];
mix?: string | string[];
title: string;
tooltip: string;
top: number;
Expand All @@ -57,6 +57,7 @@ interface PanelBaseProps {
onPositionChange: PositonChangeHandler;
onVisibilityChange: VisibilityChangeHandler;
onPositionChangeBegin: PositonChangeHandler;
children: React.ReactNode;
}

export type PanelProps = Omit<PanelBaseProps, PanelBaseExclusiveProps>
Expand Down Expand Up @@ -145,9 +146,9 @@ export const PanelBase: FC<PanelBaseProps> = ({
}, [alignment, visible, detached, resizing, locked]);

const currentIcon = useMemo(() => {
if (detached) return visible ? <IconOutlinerCollapse/> : <IconOutlinerExpand/>;
if (alignment === 'left') return visible ? <IconArrowLeft/> : <IconArrowRight/>;
if (alignment === 'right') return visible ? <IconArrowRight/> : <IconArrowLeft/>;
if (detached) return visible ? <IconOutlinerCollapse /> : <IconOutlinerExpand />;
if (alignment === 'left') return visible ? <IconArrowLeft /> : <IconArrowRight />;
if (alignment === 'right') return visible ? <IconArrowRight /> : <IconArrowLeft />;

return null;
}, [detached, visible, alignment]);
Expand Down Expand Up @@ -332,6 +333,7 @@ export const PanelBase: FC<PanelBaseProps> = ({
mod={{ enabled: visible }}
onClick={(detached && !visible) ? handleExpand : handleCollapse}
data-tooltip={tooltipText}
data-testid="panel-toggle-collapsed"
>
{currentIcon}
</Elem>
Expand Down
36 changes: 17 additions & 19 deletions src/components/SidePanels/__tests__/PanelBase.test.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
/* global test, expect, describe */
import Enzyme, { mount } from 'enzyme';
import Adapter from '@wojtekmaj/enzyme-adapter-react-17';
import { PanelBase, PanelProps } from '../PanelBase';
import { createRef } from 'react';

Enzyme.configure({ adapter: new Adapter() });
import { render } from '@testing-library/react';
import '@testing-library/jest-dom';
import userEvent from '@testing-library/user-event';

describe('PanelBase', () => {
test('Panel', async () => {
const panelName = 'details';
const rootRef = createRef<HTMLDivElement>();
let isCurrentlyExpanded = true;
const sampleProps: Partial<PanelProps> = {
const sampleProps: PanelProps = {
'top': 113,
'left': 0,
'relativeLeft': 0,
Expand All @@ -22,13 +21,13 @@ describe('PanelBase', () => {
'visible': true,
'detached': false,
'alignment': 'left',
'onResize': () => {},
'onResizeStart': () => {},
'onResizeEnd': () => {},
'onPositionChange': () => {},
'onResize': () => { },
'onResizeStart': () => { },
'onResizeEnd': () => { },
'onPositionChange': () => { },
'onVisibilityChange': (name: string, isExpanded) => { isCurrentlyExpanded = isExpanded; },
'onPositionChangeBegin': () => {},
'onSnap': () => {},
'onPositionChangeBegin': () => { },
'onSnap': () => { },
// eslint-disable-next-line
// @ts-ignore
'root': rootRef,
Expand Down Expand Up @@ -77,22 +76,21 @@ describe('PanelBase', () => {
'isDraftSaving': false,
'versions': '{draft: undefined, result: Array(0)}',
'resultSnapshot': '',
'autosave': () => {},
'autosave': () => { },
},
};
const sampleContent = 'Sample Panel';
const view = mount(<div>
{ /* eslint-disable-next-line */
/* @ts-ignore */ }
const view = render(<>
<PanelBase {...sampleProps} name={panelName} title={panelName}>
{sampleContent}
</PanelBase>
</div>);
</>);

expect(view.find('.dm-panel__title').text()).toBe(panelName);
expect(view.find('.dm-panel__body .dm-details').text()).toBe(sampleContent);
expect(view.getByText(panelName)).toBeInTheDocument();
expect(view.getByText(sampleContent)).toBeInTheDocument();
expect(isCurrentlyExpanded).toBe(true);
view.find('.dm-panel__toggle').simulate('click');

await userEvent.click(view.getByTestId('panel-toggle-collapsed'));
expect(isCurrentlyExpanded).toBe(false);
});
});
Loading