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.

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

Remove redundant if statement #21101

Conversation

E-Aho
Copy link
Contributor

@E-Aho E-Aho commented Mar 25, 2021

Summary

Fixes #20905

This if statement always would resolve to a non-null string, so would be truthy. The data was already checked for type in the switch case, so it would always be iterable. The check seems to be a leftover from a previous refactor, and is no longer needed.

Test Plan

This is just removing a redundant if statement, so the functionality is unchanged. I couldn't find any unit tests for the dehydrate function, so started to write a few, but thought that it might be out of scope of this small change. I can write some and add them to the PR if it would be helpful though!

@sizebot
Copy link

sizebot commented Mar 25, 2021

Comparing: 148f8e4...e077542

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.62 kB 122.62 kB = 39.45 kB 39.45 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.02 kB 129.02 kB = 41.43 kB 41.43 kB
facebook-www/ReactDOM-prod.classic.js = 407.89 kB 407.89 kB = 75.53 kB 75.53 kB
facebook-www/ReactDOM-prod.modern.js = 396.14 kB 396.14 kB = 73.62 kB 73.62 kB
facebook-www/ReactDOMForked-prod.classic.js = 407.89 kB 407.89 kB = 75.53 kB 75.53 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against e077542

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

This is just removing a redundant if statement, so the functionality is unchanged. I couldn't find any unit tests for the dehydrate function

FWIW there are existing tests:

it('should not dehydrate nested values until explicitly requested', async done => {
const Example = () => {
const [state] = React.useState({
foo: {
bar: {
baz: 'hi',
},
},
});
return state.foo.bar.baz;
};
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
nestedObject={{
a: {
b: {
c: [
{
d: {
e: {},
},
},
],
},
},
}}
/>,
container,
),
);
let inspectedElement = null;
let inspectElementPath = null;
// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});
async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});
inspectedElement = await inspectElementAtIndex(0);
}
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Dehydrated {
"preview_short": {…},
"preview_long": {b: {…}},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'a']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"c": Dehydrated {
"preview_short": Array(1),
"preview_long": [{…}],
},
},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'a', 'b', 'c']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"c": Array [
Object {
"d": Dehydrated {
"preview_short": {…},
"preview_long": {e: {…}},
},
},
],
},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'a', 'b', 'c', 0, 'd']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"c": Array [
Object {
"d": Object {
"e": Object {},
},
},
],
},
},
},
}
`);
await loadPath(['hooks', 0, 'value']);
expect(inspectedElement.hooks).toMatchInlineSnapshot(`
Array [
Object {
"id": 0,
"isStateEditable": true,
"name": "State",
"subHooks": Array [],
"value": Object {
"foo": Object {
"bar": Dehydrated {
"preview_short": {…},
"preview_long": {baz: "hi"},
},
},
},
},
]
`);
await loadPath(['hooks', 0, 'value', 'foo', 'bar']);
expect(inspectedElement.hooks).toMatchInlineSnapshot(`
Array [
Object {
"id": 0,
"isStateEditable": true,
"name": "State",
"subHooks": Array [],
"value": Object {
"foo": Object {
"bar": Object {
"baz": "hi",
},
},
},
},
]
`);
done();
});
it('should dehydrate complex nested values when requested', async done => {
const Example = () => null;
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
set_of_sets={new Set([new Set([1, 2, 3]), new Set(['a', 'b', 'c'])])}
/>,
container,
),
);
let inspectedElement = null;
let inspectElementPath = null;
// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});
async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});
inspectedElement = await inspectElementAtIndex(0);
}
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"set_of_sets": Object {
"0": Dehydrated {
"preview_short": Set(3),
"preview_long": Set(3) {1, 2, 3},
},
"1": Dehydrated {
"preview_short": Set(3),
"preview_long": Set(3) {"a", "b", "c"},
},
},
}
`);
await loadPath(['props', 'set_of_sets', 0]);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"set_of_sets": Object {
"0": Object {
"0": 1,
"1": 2,
"2": 3,
},
"1": Dehydrated {
"preview_short": Set(3),
"preview_long": Set(3) {"a", "b", "c"},
},
},
}
`);
done();
});
it('should include updates for nested values that were previously hydrated', async done => {
const Example = () => null;
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 1,
b: {
value: 1,
},
},
c: {
value: 1,
d: {
value: 1,
e: {
value: 1,
},
},
},
}}
/>,
container,
),
);
let inspectedElement = null;
let inspectElementPath = null;
// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});
async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});
inspectedElement = await inspectElementAtIndex(0);
}
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Dehydrated {
"preview_short": {…},
"preview_long": {b: {…}, value: 1},
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'a']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 1,
},
"value": 1,
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'c']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 1,
},
"value": 1,
},
"c": Object {
"d": Object {
"e": Dehydrated {
"preview_short": {…},
"preview_long": {value: 1},
},
"value": 1,
},
"value": 1,
},
},
}
`);
TestRendererAct(() => {
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 2,
b: {
value: 2,
},
},
c: {
value: 2,
d: {
value: 2,
e: {
value: 2,
},
},
},
}}
/>,
container,
);
});
});
// Wait for pending poll-for-update and then update inspected element data.
jest.runOnlyPendingTimers();
await Promise.resolve();
inspectedElement = await inspectElementAtIndex(0);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 2,
},
"value": 2,
},
"c": Object {
"d": Object {
"e": Dehydrated {
"preview_short": {…},
"preview_long": {value: 2},
},
"value": 2,
},
"value": 2,
},
},
}
`);
done();
});
it('should return a full update if a path is inspected for an object that has other pending changes', async done => {
const Example = () => null;
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 1,
b: {
value: 1,
},
},
c: {
value: 1,
d: {
value: 1,
e: {
value: 1,
},
},
},
}}
/>,
container,
),
);
let inspectedElement = null;
let inspectElementPath = null;
// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});
async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});
inspectedElement = await inspectElementAtIndex(0);
}
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Dehydrated {
"preview_short": {…},
"preview_long": {b: {…}, value: 1},
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);
await loadPath(['props', 'nestedObject', 'a']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 1,
},
"value": 1,
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);
TestRendererAct(() => {
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 2,
b: {
value: 2,
},
},
c: {
value: 2,
d: {
value: 2,
e: {
value: 2,
},
},
},
}}
/>,
container,
);
});
});
await loadPath(['props', 'nestedObject', 'c']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 2,
},
"value": 2,
},
"c": Object {
"d": Object {
"e": Dehydrated {
"preview_short": {…},
"preview_long": {value: 2},
},
"value": 2,
},
"value": 2,
},
},
}
`);
done();
});
it('should not tear if hydration is requested after an update', async done => {
const Example = () => null;
const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
nestedObject={{
value: 1,
a: {
value: 1,
b: {
value: 1,
},
},
}}
/>,
container,
),
);
let inspectedElement = null;
let inspectElementPath = null;
// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});
async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});
inspectedElement = await inspectElementAtIndex(0);
}
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Dehydrated {
"preview_short": {…},
"preview_long": {b: {…}, value: 1},
},
"value": 1,
},
}
`);
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
value: 2,
a: {
value: 2,
b: {
value: 2,
},
},
}}
/>,
container,
);
});
await loadPath(['props', 'nestedObject', 'a']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 2,
},
"value": 2,
},
"value": 2,
},
}
`);
done();
});

@bvaughn bvaughn merged commit a5aa9d5 into facebook:master Mar 26, 2021
@E-Aho
Copy link
Contributor Author

E-Aho commented Mar 29, 2021

Ah that makes sense. I was looking for tests which explicitly used dehydrate, so I managed to missed those.

Thanks for the help in getting this merged!

@E-Aho E-Aho deleted the Remove_redundant_condition_in_react-devtools-shared branch March 29, 2021 09:23
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 11, 2021
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 13, 2021
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 16, 2021
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 16, 2021
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 19, 2021
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant condition in react-devtools-shared
4 participants