From ca2d02da5819dfb43961b8f28378f512c688b7bd Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Mon, 14 Sep 2020 18:47:15 +0300 Subject: [PATCH 1/9] rename from iterator to iterable and split the functionality --- packages/react-devtools-shared/src/utils.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index b0b873cca163d..e29880b01ecec 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -375,7 +375,8 @@ export type DataType = | 'html_all_collection' | 'html_element' | 'infinity' - | 'iterator' + | 'iterable' + | 'opaque_iterable' | 'nan' | 'null' | 'number' @@ -435,8 +436,10 @@ export function getDataType(data: Object): DataType { // If it doesn't error, we know it's an ArrayBuffer, // but this seems kind of awkward and expensive. return 'array_buffer'; + } else if (data[Symbol.iterator]() === 'data') { + return 'opaque_iterable'; } else if (typeof data[Symbol.iterator] === 'function') { - return 'iterator'; + return 'iterable'; } else if (data.constructor && data.constructor.name === 'RegExp') { return 'regexp'; } else { @@ -613,7 +616,7 @@ export function formatDataForPreview( } else { return shortName; } - case 'iterator': + case 'iterable': const name = data.constructor.name; if (showFormattedValue) { // TRICKY @@ -653,6 +656,8 @@ export function formatDataForPreview( } else { return `${name}(${data.size})`; } + case 'opaque_iterable': + return `${name}(${data.size})`; case 'date': return data.toString(); case 'object': From 60b8bb5e1da964cbeb9d6d04be0baf1b9745f1e1 Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Fri, 18 Sep 2020 15:48:04 +0300 Subject: [PATCH 2/9] brought back the iterator variable name due to tests failing --- packages/react-devtools-shared/src/utils.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index e29880b01ecec..c605349daff45 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -375,8 +375,8 @@ export type DataType = | 'html_all_collection' | 'html_element' | 'infinity' - | 'iterable' - | 'opaque_iterable' + | 'iterator' + | 'opaque_iterator' | 'nan' | 'null' | 'number' @@ -436,10 +436,10 @@ export function getDataType(data: Object): DataType { // If it doesn't error, we know it's an ArrayBuffer, // but this seems kind of awkward and expensive. return 'array_buffer'; - } else if (data[Symbol.iterator]() === 'data') { - return 'opaque_iterable'; + } else if (data()[Symbol.iterator]() === 'data') { + return 'opaque_iterator'; } else if (typeof data[Symbol.iterator] === 'function') { - return 'iterable'; + return 'iterator'; } else if (data.constructor && data.constructor.name === 'RegExp') { return 'regexp'; } else { @@ -616,7 +616,7 @@ export function formatDataForPreview( } else { return shortName; } - case 'iterable': + case 'iterator': const name = data.constructor.name; if (showFormattedValue) { // TRICKY @@ -656,7 +656,7 @@ export function formatDataForPreview( } else { return `${name}(${data.size})`; } - case 'opaque_iterable': + case 'opaque_iterator': return `${name}(${data.size})`; case 'date': return data.toString(); From 2cf0346616a8e9c75127d7c4b15c0ad9b1c97f7f Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Fri, 18 Sep 2020 19:12:31 +0300 Subject: [PATCH 3/9] fix the issue with failing CI --- packages/react-devtools-shared/src/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index c605349daff45..37b04455fd587 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -657,7 +657,7 @@ export function formatDataForPreview( return `${name}(${data.size})`; } case 'opaque_iterator': - return `${name}(${data.size})`; + return `${data.constructor.name}(${data.size})`; case 'date': return data.toString(); case 'object': From ce81af13268ad9a02c9ddc7a3ca33810d954a123 Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Fri, 18 Sep 2020 19:22:29 +0300 Subject: [PATCH 4/9] remove parentheses before data --- packages/react-devtools-shared/src/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 37b04455fd587..8ff52e6ef88d2 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -436,7 +436,7 @@ export function getDataType(data: Object): DataType { // If it doesn't error, we know it's an ArrayBuffer, // but this seems kind of awkward and expensive. return 'array_buffer'; - } else if (data()[Symbol.iterator]() === 'data') { + } else if (data[Symbol.iterator]() === 'data') { return 'opaque_iterator'; } else if (typeof data[Symbol.iterator] === 'function') { return 'iterator'; From 25589463025311519781435213c027aef1f6b3f5 Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Fri, 18 Sep 2020 19:39:24 +0300 Subject: [PATCH 5/9] move the check in the iterator case --- packages/react-devtools-shared/src/utils.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 8ff52e6ef88d2..1e76585d4dfed 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -45,6 +45,7 @@ import { import {localStorageGetItem, localStorageSetItem} from './storage'; import {meta} from './hydration'; import type {ComponentFilter, ElementType} from './types'; +import {symbol} from 'prop-types'; const cachedDisplayNames: WeakMap = new WeakMap(); @@ -376,7 +377,6 @@ export type DataType = | 'html_element' | 'infinity' | 'iterator' - | 'opaque_iterator' | 'nan' | 'null' | 'number' @@ -436,8 +436,6 @@ export function getDataType(data: Object): DataType { // If it doesn't error, we know it's an ArrayBuffer, // but this seems kind of awkward and expensive. return 'array_buffer'; - } else if (data[Symbol.iterator]() === 'data') { - return 'opaque_iterator'; } else if (typeof data[Symbol.iterator] === 'function') { return 'iterator'; } else if (data.constructor && data.constructor.name === 'RegExp') { @@ -618,6 +616,11 @@ export function formatDataForPreview( } case 'iterator': const name = data.constructor.name; + // We check if the the generator returns itself. + // If it does, we want to avoid iterating over it. + if (typeof data[Symbol.iterator]()) { + return `${name}(${data.size})`; + } if (showFormattedValue) { // TRICKY // Don't use [...spread] syntax for this purpose. @@ -656,8 +659,6 @@ export function formatDataForPreview( } else { return `${name}(${data.size})`; } - case 'opaque_iterator': - return `${data.constructor.name}(${data.size})`; case 'date': return data.toString(); case 'object': From c62982216aef936b513ad6a1f1af5caa8ee8bd31 Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Fri, 18 Sep 2020 19:52:19 +0300 Subject: [PATCH 6/9] check if executing the function returns an object --- packages/react-devtools-shared/src/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 1e76585d4dfed..32e7af09eafa7 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -618,7 +618,7 @@ export function formatDataForPreview( const name = data.constructor.name; // We check if the the generator returns itself. // If it does, we want to avoid iterating over it. - if (typeof data[Symbol.iterator]()) { + if (typeof data[Symbol.iterator]() === 'object') { return `${name}(${data.size})`; } if (showFormattedValue) { From cf196230e87208bf49a8cd3bd159d06b89ff186a Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Tue, 22 Sep 2020 11:35:07 +0300 Subject: [PATCH 7/9] remove symbol import --- packages/react-devtools-shared/src/utils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 32e7af09eafa7..3c35fe35ad305 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -45,7 +45,6 @@ import { import {localStorageGetItem, localStorageSetItem} from './storage'; import {meta} from './hydration'; import type {ComponentFilter, ElementType} from './types'; -import {symbol} from 'prop-types'; const cachedDisplayNames: WeakMap = new WeakMap(); @@ -617,10 +616,11 @@ export function formatDataForPreview( case 'iterator': const name = data.constructor.name; // We check if the the generator returns itself. - // If it does, we want to avoid iterating over it. + // If it does, we want to avoid to iterate over it if (typeof data[Symbol.iterator]() === 'object') { return `${name}(${data.size})`; } + if (showFormattedValue) { // TRICKY // Don't use [...spread] syntax for this purpose. From f7052e8017e6a453b4a7fcc1bcaeb463762362e7 Mon Sep 17 00:00:00 2001 From: TodorTotev <51530311+TodorTotev@users.noreply.github.com> Date: Tue, 22 Sep 2020 11:38:58 +0300 Subject: [PATCH 8/9] modify the check once again --- packages/react-devtools-shared/src/utils.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 3c35fe35ad305..38a52fa05b2c8 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -376,6 +376,7 @@ export type DataType = | 'html_element' | 'infinity' | 'iterator' + | 'opaque_iterator' | 'nan' | 'null' | 'number' @@ -437,6 +438,8 @@ export function getDataType(data: Object): DataType { return 'array_buffer'; } else if (typeof data[Symbol.iterator] === 'function') { return 'iterator'; + } else if (data[Symbol.iterator] === 'data') { + return 'opaque_iterator'; } else if (data.constructor && data.constructor.name === 'RegExp') { return 'regexp'; } else { @@ -615,11 +618,6 @@ export function formatDataForPreview( } case 'iterator': const name = data.constructor.name; - // We check if the the generator returns itself. - // If it does, we want to avoid to iterate over it - if (typeof data[Symbol.iterator]() === 'object') { - return `${name}(${data.size})`; - } if (showFormattedValue) { // TRICKY @@ -659,6 +657,9 @@ export function formatDataForPreview( } else { return `${name}(${data.size})`; } + case 'opaque_iterator': { + return `${data.constructor.name}(${data.size})`; + } case 'date': return data.toString(); case 'object': From 28bb9596e7c7664b340eb8db6f02cb5a4e85b4f3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 22 Sep 2020 14:00:51 -0400 Subject: [PATCH 9/9] Added tests and fixed edge cases for DevTools iterator handling --- .../inspectedElementContext-test.js.snap | 13 +++++ .../__tests__/inspectedElementContext-test.js | 51 +++++++++++++++++++ .../__snapshots__/inspectElement-test.js.snap | 17 +++++++ .../__tests__/legacy/inspectElement-test.js | 30 +++++++++++ .../react-devtools-shared/src/hydration.js | 10 ++++ packages/react-devtools-shared/src/utils.js | 8 +-- 6 files changed, 125 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap index 1989aee6391a9..043338b8effc0 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap @@ -200,6 +200,19 @@ exports[`InspectedElementContext should inspect the currently selected element: } `; +exports[`InspectedElementContext should not consume iterables while inspecting: 1: Inspected element 2 1`] = ` +{ + "id": 2, + "owners": null, + "context": null, + "hooks": null, + "props": { + "prop": {} + }, + "state": null +} +`; + exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 1: Initially inspect element 1`] = ` { "id": 2, diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js index a68de20ab0ee1..7196ee737fa44 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js @@ -789,6 +789,57 @@ describe('InspectedElementContext', () => { done(); }); + it('should not consume iterables while inspecting', async done => { + const Example = () => null; + + function* generator() { + throw Error('Should not be consumed!'); + } + + const container = document.createElement('div'); + + const iterable = generator(); + await utils.actAsync(() => + ReactDOM.render(, container), + ); + + const id = ((store.getElementIDAtIndex(0): any): number); + + let inspectedElement = null; + + function Suspender({target}) { + const {getInspectedElement} = React.useContext(InspectedElementContext); + inspectedElement = getInspectedElement(id); + return null; + } + + await utils.actAsync( + () => + TestRenderer.create( + + + + + , + ), + false, + ); + + expect(inspectedElement).not.toBeNull(); + expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); + + const {prop} = (inspectedElement: any).props; + expect(prop[meta.inspectable]).toBe(false); + expect(prop[meta.name]).toBe('Generator'); + expect(prop[meta.type]).toBe('opaque_iterator'); + expect(prop[meta.preview_long]).toBe('Generator'); + expect(prop[meta.preview_short]).toBe('Generator'); + + done(); + }); + it('should support objects with no prototype', async done => { const Example = () => null; diff --git a/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap b/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap index 18ff7483c3f5e..46d10c4a9437b 100644 --- a/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap @@ -18,6 +18,23 @@ Object { } `; +exports[`InspectedElementContext should not consume iterables while inspecting: 1: Initial inspection 1`] = ` +Object { + "id": 2, + "type": "full-data", + "value": { + "id": 2, + "owners": null, + "context": {}, + "hooks": null, + "props": { + "iteratable": {} + }, + "state": null +}, +} +`; + exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 1: Initially inspect element 1`] = ` Object { "id": 2, diff --git a/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js b/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js index 7f1c461d93542..65330400d5846 100644 --- a/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js @@ -397,6 +397,36 @@ describe('InspectedElementContext', () => { done(); }); + it('should not consume iterables while inspecting', async done => { + const Example = () => null; + + function* generator() { + yield 1; + yield 2; + } + + const iteratable = generator(); + + act(() => + ReactDOM.render( + , + document.createElement('div'), + ), + ); + + const id = ((store.getElementIDAtIndex(0): any): number); + const inspectedElement = await read(id); + + expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); + + // Inspecting should not consume the iterable. + expect(iteratable.next().value).toEqual(1); + expect(iteratable.next().value).toEqual(2); + expect(iteratable.next().value).toBeUndefined(); + + done(); + }); + it('should support custom objects with enumerable properties and getters', async done => { class CustomData { _number = 42; diff --git a/packages/react-devtools-shared/src/hydration.js b/packages/react-devtools-shared/src/hydration.js index 2815be0f2d220..fbffd6bbd83b9 100644 --- a/packages/react-devtools-shared/src/hydration.js +++ b/packages/react-devtools-shared/src/hydration.js @@ -265,6 +265,16 @@ export function dehydrate( return unserializableValue; } + case 'opaque_iterator': + cleaned.push(path); + return { + inspectable: false, + preview_short: formatDataForPreview(data, false), + preview_long: formatDataForPreview(data, true), + name: data[Symbol.toStringTag], + type, + }; + case 'date': cleaned.push(path); return { diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 38a52fa05b2c8..bed8c125b67f4 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -437,9 +437,9 @@ export function getDataType(data: Object): DataType { // but this seems kind of awkward and expensive. return 'array_buffer'; } else if (typeof data[Symbol.iterator] === 'function') { - return 'iterator'; - } else if (data[Symbol.iterator] === 'data') { - return 'opaque_iterator'; + return data[Symbol.iterator]() === data + ? 'opaque_iterator' + : 'iterator'; } else if (data.constructor && data.constructor.name === 'RegExp') { return 'regexp'; } else { @@ -658,7 +658,7 @@ export function formatDataForPreview( return `${name}(${data.size})`; } case 'opaque_iterator': { - return `${data.constructor.name}(${data.size})`; + return data[Symbol.toStringTag]; } case 'date': return data.toString();