Skip to content

Commit

Permalink
[BUGFIX] Revert ReactDOM.render that caused some issues on the applic…
Browse files Browse the repository at this point in the history
…ation (#34)

**User-Facing Changes**
Users were unable to delete or revert a layout because the pop-up was
not functioning in development mode.

**Description**
The issue stemmed from the deprecation of ReactDOM.render. When
replaced, it caused numerous re-render issues. This PR reverts the
replacement to maintain functionality. However, a deeper review and
proper update should be conducted in the future.

**Checklist**

- [x] The web version was tested and it is running ok
- [x] The desktop version was tested and it is running ok
- [x] I've updated/created the storybook file(s)
- [x] The release version was updated on package.json files
  • Loading branch information
aneuwald-ctw authored Jul 1, 2024
1 parent 0f83366 commit a353bb5
Show file tree
Hide file tree
Showing 11 changed files with 65 additions and 60 deletions.
6 changes: 3 additions & 3 deletions benchmark/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// License, v2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";

import Logger from "@foxglove/log";
import { initI18n } from "@foxglove/studio-base";
Expand All @@ -29,8 +29,8 @@ async function main() {

const { Root } = await import("./Root");

const root = createRoot(rootEl!);
root.render(<Root />);
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(<Root />, rootEl);
}

void main();
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "foxbox",
"version": "1.0.5",
"version": "1.0.6",
"license": "MPL-2.0",
"private": true,
"productName": "Foxbox",
Expand Down
7 changes: 3 additions & 4 deletions packages/studio-base/src/components/Chart/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,10 @@ function Chart(props: Props): JSX.Element {
return;
}

// Temporarily remove setUpdateError to avoid displaying the error caused by re-rendering,
// which results in the component crashing. The crash happens because a message is sent to a
// closed RPC, causing some panels to become unusable. This approach ignores the error to
// keep the component functional. Revisit this once the underlying issue is resolved.
updateChart(newUpdate).catch((err: Error) => {
if (isMounted()) {
setUpdateError(err);
}
console.error(err);
});
}, [getNewUpdateMessage, isMounted, updateChart]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// You may not use this file except in compliance with the License.

import { SnackbarProvider } from "notistack";
import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";
import { act } from "react-dom/test-utils";

import DocumentDropListener from "@foxglove/studio-base/components/DocumentDropListener";
Expand All @@ -30,16 +30,16 @@ describe("<DocumentDropListener>", () => {
wrapper = document.createElement("div");
document.body.appendChild(wrapper);

const root = createRoot(wrapper!);

root.render(
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<div>
<SnackbarProvider>
<ThemeProvider isDark={false}>
<DocumentDropListener allowedExtensions={[]} />
</ThemeProvider>
</SnackbarProvider>
</div>,
wrapper,
);

(console.error as jest.Mock).mockClear();
Expand All @@ -62,8 +62,9 @@ describe("<DocumentDropListener>", () => {
(event as any).dataTransfer = {
types: ["Files"],
};

document.dispatchEvent(event); // The event should NOT bubble up from the document to the window
act(() => {
document.dispatchEvent(event); // The event should NOT bubble up from the document to the window
});

expect(windowDragoverHandler).not.toHaveBeenCalled();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import { StoryObj } from "@storybook/react";
import { ReactElement, useLayoutEffect, useState } from "react";
import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";

import { toSec } from "@foxglove/rostime";
import {
Expand Down Expand Up @@ -94,8 +94,8 @@ function SimplePanel({ context }: { context: PanelExtensionContext }) {
export const SimplePanelRender: StoryObj = {
render: (): ReactElement => {
function initPanel(context: PanelExtensionContext) {
const root = createRoot(context.panelElement);
root.render(<SimplePanel context={context} />);
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(<SimplePanel context={context} />, context.panelElement);
}

return (
Expand Down
33 changes: 23 additions & 10 deletions packages/studio-base/src/panels/createSyncRoot.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,50 @@
// License, v2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/
import { screen } from "@testing-library/react";
import { act } from "react-dom/test-utils";

import { createSyncRoot } from "@foxglove/studio-base/panels/createSyncRoot";

describe("createSyncRoot", () => {
const originalError = console.error;

beforeAll(() => {
// Supress specific warning about ReactDOM.render
console.error = (...args) => {
if (args[0]?.includes("Warning: ReactDOM.render is no longer supported") === true) {
return;
}
originalError.call(console, ...args);
};
});

afterAll(() => {
// Restore original console.error after tests
console.error = originalError;
});

it("should mount the component", async () => {
const textTest = "Mount Component Test";
const TestComponent = () => <div>{textTest}</div>;

const container = document.createElement("div");
document.body.appendChild(container);

act(() => {
createSyncRoot(<TestComponent />, container);
});
createSyncRoot(<TestComponent />, container);

expect(await screen.findByText(textTest)).toBeDefined();
});

it("should unmount the component", async () => {
it("should unmount the component", () => {
const textTest = "Unmount Component Test";
const TestComponent = () => <div>{textTest}</div>;

const container = document.createElement("div");
document.body.appendChild(container);

act(() => {
const unmountCb = createSyncRoot(<TestComponent />, container);
unmountCb();
});
const unmountCb = createSyncRoot(<TestComponent />, container);
expect(screen.queryAllByText(textTest)).toHaveLength(1);

expect(JSON.stringify(await screen.findByText(textTest))).toBe("{}");
unmountCb();
expect(screen.queryAllByText(textTest)).toHaveLength(0);
});
});
28 changes: 9 additions & 19 deletions packages/studio-base/src/panels/createSyncRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,26 @@
// License, v2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";

/**
* Creates a synchronized root for rendering React components.
* Creates a root for rendering React components.
*
* This function is designed to centralize the creation of React roots
* for rendering components within a given HTML element. It addresses
* potential race conditions that may occur when mounting and unmounting
* components synchronously within the React lifecycle.
* for rendering components within a given HTML element.
*
* By using a `setTimeout` with a minimal delay of 0 milliseconds, this
* function ensures that the rendering and unmounting operations occur
* asynchronously, allowing React to complete its current rendering cycle
* before proceeding with the next operation. This helps prevent race
* conditions and warnings related to synchronously unmounting roots.
*
* This approach was required since ReactDOM.render() was replaced by createRoot().render.
* This approach should be replaced by createRoot().render, but it create issues on the application.
* In the future this has to solved.
*
* @param component The JSX element to be rendered within the root.
* @param panelElement The HTML element to serve as the container for the root.
* @returns A function to unmount the root when needed.
*/
export function createSyncRoot(component: JSX.Element, panelElement: HTMLDivElement): () => void {
const root = createRoot(panelElement);
setTimeout(() => {
root.render(component);
}, 0);
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(component, panelElement);
return () => {
setTimeout(() => {
root.unmount();
}, 0);
// eslint-disable-next-line react/no-deprecated
ReactDOM.unmountComponentAtNode(panelElement);
};
}
9 changes: 4 additions & 5 deletions packages/studio-desktop/src/quicklook/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/// <reference types="quicklookjs" />

import { useState, useEffect, useRef } from "react";
import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";
import { useAsync } from "react-use";

import Logger from "@foxglove/log";
Expand Down Expand Up @@ -125,13 +125,12 @@ export function main(): void {
</div>
);
}

const root = createRoot(rootEl);

root.render(
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<>
<GlobalStyle />
<Root />
</>,
rootEl,
);
}
7 changes: 4 additions & 3 deletions packages/studio-desktop/src/renderer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/// <reference types="electron" />

import { StrictMode, useEffect } from "react";
import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";

import { Sockets } from "@foxglove/electron-socket/renderer";
import Logger from "@foxglove/log";
Expand Down Expand Up @@ -58,8 +58,8 @@ export async function main(params: MainParams): Promise<void> {
await waitForFonts();
await initI18n();

const root = createRoot(rootEl);
root.render(
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<StrictMode>
<LogAfterRender>
<Root
Expand All @@ -69,5 +69,6 @@ export async function main(params: MainParams): Promise<void> {
/>
</LogAfterRender>
</StrictMode>,
rootEl,
);
}
12 changes: 7 additions & 5 deletions packages/studio-web/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import { StrictMode, useEffect } from "react";
import { createRoot } from "react-dom/client";
import ReactDOM from "react-dom";

import Logger from "@foxglove/log";
import type { IDataSourceFactory } from "@foxglove/studio-base";
Expand Down Expand Up @@ -41,8 +41,6 @@ export async function main(getParams: () => Promise<MainParams> = async () => ({
throw new Error("missing #root element");
}

const root = createRoot(rootEl);

const chromeMatch = navigator.userAgent.match(/Chrome\/(\d+)\./);
const chromeVersion = chromeMatch ? parseInt(chromeMatch[1] ?? "", 10) : 0;
const isChrome = chromeVersion !== 0;
Expand All @@ -57,12 +55,14 @@ export async function main(getParams: () => Promise<MainParams> = async () => ({
);

if (!canRender) {
root.render(
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<StrictMode>
<LogAfterRender>
<CssBaseline>{banner}</CssBaseline>
</LogAfterRender>
</StrictMode>,
rootEl,
);
return;
}
Expand All @@ -85,12 +85,14 @@ export async function main(getParams: () => Promise<MainParams> = async () => ({
</WebRoot>
);

root.render(
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<StrictMode>
<LogAfterRender>
{banner}
{rootElement}
</LogAfterRender>
</StrictMode>,
rootEl,
);
}
2 changes: 1 addition & 1 deletion packages/studio/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@foxglove/studio",
"version": "1.0.5",
"version": "1.0.6",
"license": "MPL-2.0",
"repository": {
"type": "git",
Expand Down

0 comments on commit a353bb5

Please sign in to comment.