From a353bb597ec2611cf15b83683fc175cf393e68b9 Mon Sep 17 00:00:00 2001 From: Alexandre Neuwald CTW <162589552+aneuwald-ctw@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:57:43 +0100 Subject: [PATCH] [BUGFIX] Revert ReactDOM.render that caused some issues on the application (#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 --- benchmark/src/index.tsx | 6 ++-- package.json | 2 +- .../src/components/Chart/index.tsx | 7 ++-- .../components/DocumentDropListener.test.tsx | 13 ++++---- .../PanelExtensionAdapter.stories.tsx | 6 ++-- .../src/panels/createSyncRoot.test.tsx | 33 +++++++++++++------ .../studio-base/src/panels/createSyncRoot.tsx | 28 +++++----------- .../studio-desktop/src/quicklook/index.tsx | 9 +++-- .../studio-desktop/src/renderer/index.tsx | 7 ++-- packages/studio-web/src/index.tsx | 12 ++++--- packages/studio/package.json | 2 +- 11 files changed, 65 insertions(+), 60 deletions(-) diff --git a/benchmark/src/index.tsx b/benchmark/src/index.tsx index ca67bd71fd..0da0756ccc 100644 --- a/benchmark/src/index.tsx +++ b/benchmark/src/index.tsx @@ -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"; @@ -29,8 +29,8 @@ async function main() { const { Root } = await import("./Root"); - const root = createRoot(rootEl!); - root.render(); + // eslint-disable-next-line react/no-deprecated + ReactDOM.render(, rootEl); } void main(); diff --git a/package.json b/package.json index d3dea08601..43a90a7534 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "foxbox", - "version": "1.0.5", + "version": "1.0.6", "license": "MPL-2.0", "private": true, "productName": "Foxbox", diff --git a/packages/studio-base/src/components/Chart/index.tsx b/packages/studio-base/src/components/Chart/index.tsx index 40cc66750d..8feaa69cb1 100644 --- a/packages/studio-base/src/components/Chart/index.tsx +++ b/packages/studio-base/src/components/Chart/index.tsx @@ -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]); diff --git a/packages/studio-base/src/components/DocumentDropListener.test.tsx b/packages/studio-base/src/components/DocumentDropListener.test.tsx index 588c8c919c..1ff509460d 100644 --- a/packages/studio-base/src/components/DocumentDropListener.test.tsx +++ b/packages/studio-base/src/components/DocumentDropListener.test.tsx @@ -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"; @@ -30,9 +30,8 @@ describe("", () => { wrapper = document.createElement("div"); document.body.appendChild(wrapper); - const root = createRoot(wrapper!); - - root.render( + // eslint-disable-next-line react/no-deprecated + ReactDOM.render(
@@ -40,6 +39,7 @@ describe("", () => {
, + wrapper, ); (console.error as jest.Mock).mockClear(); @@ -62,8 +62,9 @@ describe("", () => { (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(); }); diff --git a/packages/studio-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.stories.tsx b/packages/studio-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.stories.tsx index d9b041d084..fca01ff43d 100644 --- a/packages/studio-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.stories.tsx +++ b/packages/studio-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.stories.tsx @@ -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 { @@ -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(); + // eslint-disable-next-line react/no-deprecated + ReactDOM.render(, context.panelElement); } return ( diff --git a/packages/studio-base/src/panels/createSyncRoot.test.tsx b/packages/studio-base/src/panels/createSyncRoot.test.tsx index 6bde2939c4..81bdf8fe03 100644 --- a/packages/studio-base/src/panels/createSyncRoot.test.tsx +++ b/packages/studio-base/src/panels/createSyncRoot.test.tsx @@ -3,11 +3,27 @@ // 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 = () =>
{textTest}
; @@ -15,25 +31,22 @@ describe("createSyncRoot", () => { const container = document.createElement("div"); document.body.appendChild(container); - act(() => { - createSyncRoot(, container); - }); + createSyncRoot(, 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 = () =>
{textTest}
; const container = document.createElement("div"); document.body.appendChild(container); - act(() => { - const unmountCb = createSyncRoot(, container); - unmountCb(); - }); + const unmountCb = createSyncRoot(, container); + expect(screen.queryAllByText(textTest)).toHaveLength(1); - expect(JSON.stringify(await screen.findByText(textTest))).toBe("{}"); + unmountCb(); + expect(screen.queryAllByText(textTest)).toHaveLength(0); }); }); diff --git a/packages/studio-base/src/panels/createSyncRoot.tsx b/packages/studio-base/src/panels/createSyncRoot.tsx index 119bb85ada..ccc08888f5 100644 --- a/packages/studio-base/src/panels/createSyncRoot.tsx +++ b/packages/studio-base/src/panels/createSyncRoot.tsx @@ -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); }; } diff --git a/packages/studio-desktop/src/quicklook/index.tsx b/packages/studio-desktop/src/quicklook/index.tsx index 250ef3a1fb..1228ce2390 100644 --- a/packages/studio-desktop/src/quicklook/index.tsx +++ b/packages/studio-desktop/src/quicklook/index.tsx @@ -5,7 +5,7 @@ /// 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"; @@ -125,13 +125,12 @@ export function main(): void { ); } - - const root = createRoot(rootEl); - - root.render( + // eslint-disable-next-line react/no-deprecated + ReactDOM.render( <> , + rootEl, ); } diff --git a/packages/studio-desktop/src/renderer/index.tsx b/packages/studio-desktop/src/renderer/index.tsx index e86d6b6d0b..b134ac2179 100644 --- a/packages/studio-desktop/src/renderer/index.tsx +++ b/packages/studio-desktop/src/renderer/index.tsx @@ -6,7 +6,7 @@ /// 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"; @@ -58,8 +58,8 @@ export async function main(params: MainParams): Promise { await waitForFonts(); await initI18n(); - const root = createRoot(rootEl); - root.render( + // eslint-disable-next-line react/no-deprecated + ReactDOM.render( { /> , + rootEl, ); } diff --git a/packages/studio-web/src/index.tsx b/packages/studio-web/src/index.tsx index f8a47eeb50..de3da8b278 100644 --- a/packages/studio-web/src/index.tsx +++ b/packages/studio-web/src/index.tsx @@ -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"; @@ -41,8 +41,6 @@ export async function main(getParams: () => Promise = 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; @@ -57,12 +55,14 @@ export async function main(getParams: () => Promise = async () => ({ ); if (!canRender) { - root.render( + // eslint-disable-next-line react/no-deprecated + ReactDOM.render( {banner} , + rootEl, ); return; } @@ -85,12 +85,14 @@ export async function main(getParams: () => Promise = async () => ({ ); - root.render( + // eslint-disable-next-line react/no-deprecated + ReactDOM.render( {banner} {rootElement} , + rootEl, ); } diff --git a/packages/studio/package.json b/packages/studio/package.json index 770eadf15a..ce4e38dd30 100644 --- a/packages/studio/package.json +++ b/packages/studio/package.json @@ -1,6 +1,6 @@ { "name": "@foxglove/studio", - "version": "1.0.5", + "version": "1.0.6", "license": "MPL-2.0", "repository": { "type": "git",