Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[devtools] Fix can't expand prop value in some scenario #20534

Merged
merged 4 commits into from
Jan 19, 2021
Merged

[devtools] Fix can't expand prop value in some scenario #20534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8111b2
[devtools] Fix can't expand in some scenario
iChenLei Jan 2, 2021
7627c9a
[devtools] Fix can't expand prop value in some scenarios
iChenLei Jan 5, 2021
77f9b70
Merge branch 'devtools-cant-expand' of github.com:iChenLei/react into…
Jan 19, 2021
8290732
Merged master and resolved conflicts
Jan 19, 2021
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
77 changes: 77 additions & 0 deletions packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,83 @@ describe('InspectedElement', () => {
done();
});

it('should allow component prop value and value`s prototype has same name params.', async done => {
const testData = Object.create(
{
a: undefined,
b: Infinity,
c: NaN,
d: 'normal',
},
{
a: {
value: undefined,
writable: true,
enumerable: true,
configurable: true,
},
b: {
value: Infinity,
writable: true,
enumerable: true,
configurable: true,
},
c: {
value: NaN,
writable: true,
enumerable: true,
configurable: true,
},
d: {
value: 'normal',
writable: true,
enumerable: true,
configurable: true,
},
},
);
const Example = ({data}) => null;
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(<Example data={testData} />, container),
);

const id = ((store.getElementIDAtIndex(0): any): number);

let inspectedElement = null;

function Suspender({target}) {
inspectedElement = useInspectedElement(target);
return null;
}

await utils.actAsync(
() =>
TestRenderer.create(
<Contexts
defaultSelectedElementID={id}
defaultSelectedElementIndex={0}>
<React.Suspense fallback={null}>
<Suspender target={id} />
</React.Suspense>
</Contexts>,
),
false,
);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"data": Object {
"a": undefined,
"b": Infinity,
"c": NaN,
"d": "normal",
},
}
`);

done();
});

it('should not dehydrate nested values until explicitly requested', async done => {
const Example = () => {
const [state] = React.useState({
Expand Down
60 changes: 60 additions & 0 deletions packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,66 @@ describe('InspectedElementContext', () => {
done();
});

it('should allow component prop value and value`s prototype has same name params.', async done => {
const testData = Object.create(
{
a: undefined,
b: Infinity,
c: NaN,
d: 'normal',
},
{
a: {
value: undefined,
writable: true,
enumerable: true,
configurable: true,
},
b: {
value: Infinity,
writable: true,
enumerable: true,
configurable: true,
},
c: {
value: NaN,
writable: true,
enumerable: true,
configurable: true,
},
d: {
value: 'normal',
writable: true,
enumerable: true,
configurable: true,
},
},
);

const Example = ({data}) => null;
act(() =>
ReactDOM.render(
<Example data={testData} />,
document.createElement('div'),
),
);

const id = ((store.getElementIDAtIndex(0): any): number);
const inspectedElement = await read(id);

expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"data": Object {
"a": undefined,
"b": Infinity,
"c": NaN,
"d": "normal",
},
}
`);
done();
});

it('should not dehydrate nested values until explicitly requested', async done => {
const Example = () => null;

Expand Down
4 changes: 3 additions & 1 deletion packages/react-devtools-shared/src/hydration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,9 @@ export function hydrate(

const value = parent[last];

if (value.type === 'infinity') {
if (!value) {
return;
} else if (value.type === 'infinity') {
parent[last] = Infinity;
} else if (value.type === 'nan') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can have same issue for Infinty ?

Copy link
Contributor Author

@iChenLei iChenLei Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I miss the scenario which value is Infinity(Infinity is Truthy). Any suggestion for me ? I think i did not understand your PR #19786 (I mean all logic detail) is the biggest obstacle to fix this bug.

- cleaned.forEach((path: Array<string | number>) => {
+ [...new Set(cleaned)].forEach((path: Array<string | number>) => {

Is these code ok? When we loop cleaned array, we filter the duplicate item and don't need check the parent[last]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have duplicate keys here actually. In your example I think we have two symbols with the same name and as I have to change the symbols into a string so that we can do the postmessage that's why we have this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write any Symbol in my business code, Just class(decorators) 、mobx and react. I think store and store prototype has same params is the reason. (mobx internal reactivity machine logic)
Mobx will add @observable params to the prototype, so getAllEnumerableKeys will return [..., 'finished', 'finished'].

Look the codesandbox console or chrome devtools console
Reproduce Codesandbox link

Copy link
Contributor

@omarsy omarsy Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I can now reproduce the issue with a basic object.
Reproduce issue
Maybe @bvaughn we should not include inherited keys or find a way to avoid duplicates ?
Or you think it is normal to have this ?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I rebase my PR with your code 😆 , and add unit test later. ( Now it's 02:16 in shanghai, china. I need to sleep. Thank u, bvaughn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rest! 🙇

Ping me once you've added a test and I'll review again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bvaughn Hi, I had finished the unit test, please review, thank u very much ! 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It's back in my queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no problem in this pull request, I hope you can release a new small version (eg: 4.10.2) for react devtools , that's very helpful for my team. Thank you ! bvaughn

parent[last] = NaN;
Expand Down
8 changes: 4 additions & 4 deletions packages/react-devtools-shared/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ export function alphaSortKeys(

export function getAllEnumerableKeys(
obj: Object,
): Array<string | number | Symbol> {
const keys = [];
): Set<string | number | Symbol> {
const keys = new Set();
let current = obj;
while (current != null) {
const currentKeys = [
Expand All @@ -82,7 +82,7 @@ export function getAllEnumerableKeys(
currentKeys.forEach(key => {
// $FlowFixMe: key can be a Symbol https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptor
if (descriptors[key].enumerable) {
keys.push(key);
keys.add(key);
}
});
current = Object.getPrototypeOf(current);
Expand Down Expand Up @@ -767,7 +767,7 @@ export function formatDataForPreview(
return data.toString();
case 'object':
if (showFormattedValue) {
const keys = getAllEnumerableKeys(data).sort(alphaSortKeys);
const keys = Array.from(getAllEnumerableKeys(data)).sort(alphaSortKeys);

let formatted = '';
for (let i = 0; i < keys.length; i++) {
Expand Down