-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Remove redundant if statement #21101
Conversation
Comparing: 148f8e4...e077542 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
There was a problem hiding this 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:
react/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
Lines 980 to 1603 in a77dd13
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(); | |
}); |
Ah that makes sense. I was looking for tests which explicitly used Thanks for the help in getting this merged! |
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!