From a3152eda5f89e20f056521855f7fa101ce50e4c3 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Wed, 8 Feb 2023 11:32:38 -0800 Subject: [PATCH 1/2] support ReactDOM.render(..., document) without crashing (#26129) as reported in #26128 `ReactDOM.render(..., document)` crashed when `enableHostSingletons` was on. This is because it had a different way of clearing the container than `createRoot(document)`. I updated the legacy implementation to share the clearing behavior of `creatRoot` which will preserve the singleton instances. I also removed the warning saying not to use `document.body` as a container --- .../ReactDOMSingletonComponents-test.js | 17 +++++++++++++++++ .../react-dom/src/__tests__/ReactMount-test.js | 17 +++++++++++------ .../src/__tests__/validateDOMNesting-test.js | 18 +++++++++--------- .../react-dom/src/client/ReactDOMLegacy.js | 8 ++++---- 4 files changed, 41 insertions(+), 19 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js b/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js index 026302c18cc27..f78a7290b9c90 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js @@ -13,6 +13,7 @@ let JSDOM; let Stream; let Scheduler; let React; +let ReactDOM; let ReactDOMClient; let ReactDOMFizzServer; let document; @@ -28,6 +29,7 @@ describe('ReactDOM HostSingleton', () => { JSDOM = require('jsdom').JSDOM; Scheduler = require('scheduler'); React = require('react'); + ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); ReactDOMFizzServer = require('react-dom/server'); Stream = require('stream'); @@ -1007,4 +1009,19 @@ describe('ReactDOM HostSingleton', () => { , ); }); + + // https://github.com/facebook/react/issues/26128 + it('(#26128) does not throw when rendering at body', async () => { + ReactDOM.render(
, document.body); + }); + + // https://github.com/facebook/react/issues/26128 + it('(#26128) does not throw when rendering at ', async () => { + ReactDOM.render(, document.documentElement); + }); + + // https://github.com/facebook/react/issues/26128 + it('(#26128) does not throw when rendering at document', async () => { + ReactDOM.render(, document); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactMount-test.js b/packages/react-dom/src/__tests__/ReactMount-test.js index c69ed03559a66..f383bec90f3cb 100644 --- a/packages/react-dom/src/__tests__/ReactMount-test.js +++ b/packages/react-dom/src/__tests__/ReactMount-test.js @@ -149,12 +149,17 @@ describe('ReactMount', () => { const iFrame = document.createElement('iframe'); document.body.appendChild(iFrame); - expect(() => - ReactDOM.render(
, iFrame.contentDocument.body), - ).toErrorDev( - 'Rendering components directly into document.body is discouraged', - {withoutStack: true}, - ); + if (gate(flags => flags.enableHostSingletons)) { + // HostSingletons make the warning for document.body unecessary + ReactDOM.render(
, iFrame.contentDocument.body); + } else { + expect(() => + ReactDOM.render(
, iFrame.contentDocument.body), + ).toErrorDev( + 'Rendering components directly into document.body is discouraged', + {withoutStack: true}, + ); + } }); it('should account for escaping on a checksum mismatch', () => { diff --git a/packages/react-dom/src/__tests__/validateDOMNesting-test.js b/packages/react-dom/src/__tests__/validateDOMNesting-test.js index f6ec407ff9bc3..ca1d921134785 100644 --- a/packages/react-dom/src/__tests__/validateDOMNesting-test.js +++ b/packages/react-dom/src/__tests__/validateDOMNesting-test.js @@ -28,9 +28,11 @@ function expectWarnings(tags, warnings = [], withoutStack = 0) { element = {element}; } - expect(() => ReactDOM.render(element, container)).toErrorDev(warnings, { - withoutStack, - }); + if (warnings.length) { + expect(() => ReactDOM.render(element, container)).toErrorDev(warnings, { + withoutStack, + }); + } } describe('validateDOMNesting', () => { @@ -39,8 +41,10 @@ describe('validateDOMNesting', () => { expectWarnings( ['body', 'datalist', 'option'], [ - 'render(): Rendering components directly into document.body is discouraged', - ], + gate(flags => !flags.enableHostSingletons) + ? 'render(): Rendering components directly into document.body is discouraged' + : null, + ].filter(Boolean), 1, ); expectWarnings(['div', 'a', 'object', 'a']); @@ -106,13 +110,9 @@ describe('validateDOMNesting', () => { expectWarnings( ['body', 'body'], [ - 'render(): Rendering components directly into document.body is discouraged', 'validateDOMNesting(...): cannot appear as a child of .\n' + ' in body (at **)', - 'Warning: You are mounting a new body component when a previous one has not first unmounted. It is an error to render more than one body component at a time and attributes and children of these components will likely fail in unpredictable ways. Please only render a single instance of and if you need to mount a new one, ensure any previous ones have unmounted first.\n' + - ' in body (at **)', ], - 1, ); } else { expectWarnings( diff --git a/packages/react-dom/src/client/ReactDOMLegacy.js b/packages/react-dom/src/client/ReactDOMLegacy.js index b1f06e6e15dbe..d2125b9adab02 100644 --- a/packages/react-dom/src/client/ReactDOMLegacy.js +++ b/packages/react-dom/src/client/ReactDOMLegacy.js @@ -14,6 +14,7 @@ import type { import type {FiberRoot} from 'react-reconciler/src/ReactInternalTypes'; import type {ReactNodeList} from 'shared/ReactTypes'; +import {clearContainer} from 'react-dom-bindings/src/client/ReactDOMHostConfig'; import { getInstanceFromNode, isContainerMarkedAsRoot, @@ -42,6 +43,7 @@ import {LegacyRoot} from 'react-reconciler/src/ReactRootTags'; import getComponentNameFromType from 'shared/getComponentNameFromType'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {has as hasInstance} from 'shared/ReactInstanceMap'; +import {enableHostSingletons} from '../../../shared/ReactFeatureFlags'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -79,6 +81,7 @@ if (__DEV__) { } if ( + !enableHostSingletons && container.nodeType === ELEMENT_NODE && ((container: any): Element).tagName && ((container: any): Element).tagName.toUpperCase() === 'BODY' @@ -152,10 +155,7 @@ function legacyCreateRootFromDOMContainer( return root; } else { // First clear any existing content. - let rootSibling; - while ((rootSibling = container.lastChild)) { - container.removeChild(rootSibling); - } + clearContainer(container); if (typeof callback === 'function') { const originalCallback = callback; From 78d2e9e2a894a7ea9aa3f9faadfc4c6038e86a75 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Wed, 8 Feb 2023 20:00:22 -0500 Subject: [PATCH 2/2] Replace DevTools `semver` usages with `compare-versions` for smaller bundle size (#26122) ## Summary This PR: - Replaces the existing usages of methods from the `semver` library in the React DevTools source with an inlined version based on https://www.npmjs.com/package/semver-compare. This appears to drop the unminified bundle sizes of 3 separate `react-devtools-extensions` build artifacts by about 50K: ![image](https://user-images.githubusercontent.com/1128784/217326947-4c26d1be-d834-4f77-9e6e-be2d5ed0954d.png) ## How did you test this change? I was originally working on [a fork of React DevTools](https://github.com/replayio/react/pull/2) for use with https://replay.io , specifically our integration of the React DevTools UI to show the React component tree while users are debugging a recorded application. As part of the dev work on that fork, I wanted to shrink the bundle size of the extension's generated JS build artifacts. I noted that the official NPM `semver` library was taking up a noticeable chunk of space in the bundles, and saw that it's only being used in a handful of places to do some very simple version string comparisons. I was able to replace the `semver` imports and usages with a simple alternate comparison function, and confirmed via hands-on checks and console logging that the checks behaved the same way. Given that, I wanted to upstream this particular change to help shrink the real extension's bundle sizes. I know that it's an extension, so bundle size isn't _as_ critical a concern as it would be for a pure library. But, smaller download sizes do benefit all users, and that also includes sites like CodeSandbox and Replay that are using the React DevTools as a library as well. I'm happy to tweak this PR if necessary. Thanks! --- packages/react-devtools-shared/package.json | 4 ++-- .../src/__tests__/utils-test.js | 16 ++++++++++++++++ .../src/backend/renderer.js | 2 +- .../react-devtools-shared/src/backend/utils.js | 9 +++++++++ .../src/e2e-regression/app-legacy.js | 3 ++- yarn.lock | 5 +++++ 6 files changed, 35 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/package.json b/packages/react-devtools-shared/package.json index 9d15ae7869d45..b03ce9ea74310 100644 --- a/packages/react-devtools-shared/package.json +++ b/packages/react-devtools-shared/package.json @@ -18,11 +18,11 @@ "@reach/menu-button": "^0.16.1", "@reach/tooltip": "^0.16.0", "clipboard-js": "^0.3.6", + "compare-versions": "^5.0.3", "json5": "^2.1.3", "local-storage-fallback": "^4.1.1", "lodash.throttle": "^4.1.1", "memoize-one": "^3.1.1", - "react-virtualized-auto-sizer": "^1.0.6", - "semver": "^6.3.0" + "react-virtualized-auto-sizer": "^1.0.6" } } diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index ecb2cac33eb2a..5cb94fc051cc0 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/packages/react-devtools-shared/src/__tests__/utils-test.js @@ -15,6 +15,8 @@ import {stackToComponentSources} from 'react-devtools-shared/src/devtools/utils' import { format, formatWithStyles, + gt, + gte, } from 'react-devtools-shared/src/backend/utils'; import { REACT_SUSPENSE_LIST_TYPE as SuspenseList, @@ -252,4 +254,18 @@ describe('utils', () => { ]); }); }); + + describe('semver comparisons', () => { + it('gte should compare versions correctly', () => { + expect(gte('1.2.3', '1.2.1')).toBe(true); + expect(gte('1.2.1', '1.2.1')).toBe(true); + expect(gte('1.2.1', '1.2.2')).toBe(false); + }); + + it('gt should compare versions correctly', () => { + expect(gt('1.2.3', '1.2.1')).toBe(true); + expect(gt('1.2.1', '1.2.1')).toBe(false); + expect(gt('1.2.1', '1.2.2')).toBe(false); + }); + }); }); diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 1028e35726cad..cf2660fc690f3 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -7,7 +7,6 @@ * @flow */ -import {gt, gte} from 'semver'; import { ComponentFilterDisplayName, ComponentFilterElementType, @@ -39,6 +38,7 @@ import { utfEncodeString, } from 'react-devtools-shared/src/utils'; import {sessionStorageGetItem} from 'react-devtools-shared/src/storage'; +import {gt, gte} from 'react-devtools-shared/src/backend/utils'; import { cleanForBridge, copyToClipboard, diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index be5bfa56a869a..7ae15e173dfbb 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -9,6 +9,7 @@ */ import {copy} from 'clipboard-js'; +import {compareVersions} from 'compare-versions'; import {dehydrate} from '../hydration'; import isArray from 'shared/isArray'; @@ -275,3 +276,11 @@ export function isSynchronousXHRSupported(): boolean { window.document.featurePolicy.allowsFeature('sync-xhr') ); } + +export function gt(a: string = '', b: string = ''): boolean { + return compareVersions(a, b) === 1; +} + +export function gte(a: string = '', b: string = ''): boolean { + return compareVersions(a, b) > -1; +} diff --git a/packages/react-devtools-shell/src/e2e-regression/app-legacy.js b/packages/react-devtools-shell/src/e2e-regression/app-legacy.js index dedbed5e73f7a..e8bb10fda091b 100644 --- a/packages/react-devtools-shell/src/e2e-regression/app-legacy.js +++ b/packages/react-devtools-shell/src/e2e-regression/app-legacy.js @@ -4,9 +4,10 @@ import * as React from 'react'; import * as ReactDOM from 'react-dom'; -import {gte} from 'semver'; import ListApp from '../e2e-apps/ListApp'; import ListAppLegacy from '../e2e-apps/ListAppLegacy'; +import {gte} from 'react-devtools-shared/src/backend/utils'; + const version = process.env.E2E_APP_REACT_VERSION; function mountApp(App: () => React$Node) { diff --git a/yarn.lock b/yarn.lock index 37be19fac609c..966bfe2fa541f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5458,6 +5458,11 @@ commondir@^1.0.1: resolved "https://registry.yarnpkg.com/commondir/-/commondir-1.0.1.tgz#ddd800da0c66127393cca5950ea968a3aaf1253b" integrity sha1-3dgA2gxmEnOTzKWVDqloo6rxJTs= +compare-versions@^5.0.3: + version "5.0.3" + resolved "https://registry.yarnpkg.com/compare-versions/-/compare-versions-5.0.3.tgz#a9b34fea217472650ef4a2651d905f42c28ebfd7" + integrity sha512-4UZlZP8Z99MGEY+Ovg/uJxJuvoXuN4M6B3hKaiackiHrgzQFEe3diJi1mf1PNHbFujM7FvLrK2bpgIaImbtZ1A== + component-emitter@^1.2.1: version "1.3.0" resolved "https://registry.yarnpkg.com/component-emitter/-/component-emitter-1.3.0.tgz#16e4070fba8ae29b679f2215853ee181ab2eabc0"