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

Always keep disabled logs in the second pass #21739

Merged
merged 4 commits into from
Jun 24, 2021
Merged

Always keep disabled logs in the second pass #21739

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
0992d09
Add tests for disabled logs
gaearon Jun 23, 2021
aec503f
Always keep disabled logs in the second pass
gaearon Jun 23, 2021
96d2a7b
Jest nit
gaearon Jun 24, 2021
adfa3d4
Always use the second result
gaearon Jun 24, 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
32 changes: 14 additions & 18 deletions packages/react-reconciler/src/ReactFiberClassComponent.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
nextProps: any,
) {
const prevState = workInProgress.memoizedState;

let partialState = getDerivedStateFromProps(nextProps, prevState);
if (__DEV__) {
if (
debugRenderPhaseSideEffectsForStrictMode &&
Expand All @@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
disableLogs();
try {
// Invoke the function an extra time to help detect side-effects.
getDerivedStateFromProps(nextProps, prevState);
partialState = getDerivedStateFromProps(nextProps, prevState);
} finally {
reenableLogs();
}
}
}

const partialState = getDerivedStateFromProps(nextProps, prevState);

if (__DEV__) {
warnOnUndefinedDerivedState(ctor, partialState);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so this PR does more than just disable logs during the second pass. It also changes the code to use the result of the first pass rather than the second pass? Should be okay so long as there are no unsafe side effects?

I think that using the 2nd call (for this and other methods below) may have been an intentional choice in case there were side effects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. I haven't thought about this. I do vaguely remember some two year old conversations about this. Maybe let's hold off merging and keep it for the next sync discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool beans 👍🏼

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the double rendering order in general is very intentional eg renderWithHooks.

However I noticed when doing the Fizz parts that some of the class ones are reverse order. I'm not sure that was intentional or an oversight when we intentionally did the other ones.

Because I think there are some subtle semantic issues with things like memoization in general that's an issue here. Deduping warnings being one such memoization that breaks.

Copy link
Collaborator Author

@gaearon gaearon Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The way we do it with Hooks is we disable logs during the second one and use the result of the second one. I could make everything match that.

Copy link
Contributor

@tjallingt tjallingt Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've been extremely confused before why the logs and the returned values didn't match... Maybe disabling the logs on the first call and enable logs and use the result of the second call? On the other hand that might make it even harder to deduce whats happening since you can't "see" the first call at all...

// Merge the partial state and the previous state.
Expand Down Expand Up @@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
) {
const instance = workInProgress.stateNode;
if (typeof instance.shouldComponentUpdate === 'function') {
let shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);
if (__DEV__) {
if (
debugRenderPhaseSideEffectsForStrictMode &&
Expand All @@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
disableLogs();
try {
// Invoke the function an extra time to help detect side-effects.
instance.shouldComponentUpdate(newProps, newState, nextContext);
shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);
} finally {
reenableLogs();
}
}
}
const shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);

if (__DEV__) {
if (shouldUpdate === undefined) {
console.error(
'%s.shouldComponentUpdate(): Returned undefined instead of a ' +
Expand Down Expand Up @@ -659,6 +655,7 @@ function constructClassInstance(
: emptyContextObject;
}

let instance = new ctor(props, context);
// Instantiate twice to help detect side-effects.
if (__DEV__) {
if (
Expand All @@ -667,14 +664,13 @@ function constructClassInstance(
) {
disableLogs();
try {
new ctor(props, context); // eslint-disable-line no-new
instance = new ctor(props, context); // eslint-disable-line no-new
} finally {
reenableLogs();
}
}
}

const instance = new ctor(props, context);
const state = (workInProgress.memoizedState =
instance.state !== null && instance.state !== undefined
? instance.state
Expand Down
32 changes: 14 additions & 18 deletions packages/react-reconciler/src/ReactFiberClassComponent.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
nextProps: any,
) {
const prevState = workInProgress.memoizedState;

let partialState = getDerivedStateFromProps(nextProps, prevState);
if (__DEV__) {
if (
debugRenderPhaseSideEffectsForStrictMode &&
Expand All @@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
disableLogs();
try {
// Invoke the function an extra time to help detect side-effects.
getDerivedStateFromProps(nextProps, prevState);
partialState = getDerivedStateFromProps(nextProps, prevState);
} finally {
reenableLogs();
}
}
}

const partialState = getDerivedStateFromProps(nextProps, prevState);

if (__DEV__) {
warnOnUndefinedDerivedState(ctor, partialState);
}
// Merge the partial state and the previous state.
Expand Down Expand Up @@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
) {
const instance = workInProgress.stateNode;
if (typeof instance.shouldComponentUpdate === 'function') {
let shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);
if (__DEV__) {
if (
debugRenderPhaseSideEffectsForStrictMode &&
Expand All @@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
disableLogs();
try {
// Invoke the function an extra time to help detect side-effects.
instance.shouldComponentUpdate(newProps, newState, nextContext);
shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);
} finally {
reenableLogs();
Copy link
Contributor

@behnammodi behnammodi Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think reEnableLogs is better than reenableLogs

}
}
}
const shouldUpdate = instance.shouldComponentUpdate(
newProps,
newState,
nextContext,
);

if (__DEV__) {
if (shouldUpdate === undefined) {
console.error(
'%s.shouldComponentUpdate(): Returned undefined instead of a ' +
Expand Down Expand Up @@ -659,6 +655,7 @@ function constructClassInstance(
: emptyContextObject;
}

let instance = new ctor(props, context);
// Instantiate twice to help detect side-effects.
if (__DEV__) {
if (
Expand All @@ -667,14 +664,13 @@ function constructClassInstance(
) {
disableLogs();
try {
new ctor(props, context); // eslint-disable-line no-new
instance = new ctor(props, context); // eslint-disable-line no-new
} finally {
reenableLogs();
}
}
}

const instance = new ctor(props, context);
const state = (workInProgress.memoizedState =
instance.state !== null && instance.state !== undefined
? instance.state
Expand Down
192 changes: 192 additions & 0 deletions packages/react/src/__tests__/ReactStrictMode-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,4 +892,196 @@ describe('context legacy', () => {
// Dedupe
ReactDOM.render(<Root />, container);
});

describe('disableLogs', () => {
it('disables logs once for class double render', () => {
spyOnDevAndProd(console, 'log');

let count = 0;
class Foo extends React.Component {
render() {
count++;
console.log('foo ' + count);
return null;
}
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests 👍🏼


it('disables logs once for class double ctor', () => {
spyOnDevAndProd(console, 'log');

let count = 0;
class Foo extends React.Component {
constructor(props) {
super(props);
count++;
console.log('foo ' + count);
}
render() {
return null;
}
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});

it('disables logs once for class double getDerivedStateFromProps', () => {
spyOnDevAndProd(console, 'log');

let count = 0;
class Foo extends React.Component {
state = {};
static getDerivedStateFromProps() {
count++;
console.log('foo ' + count);
return {};
}
render() {
return null;
}
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});

it('disables logs once for class double shouldComponentUpdate', () => {
spyOnDevAndProd(console, 'log');

let count = 0;
class Foo extends React.Component {
state = {};
shouldComponentUpdate() {
count++;
console.log('foo ' + count);
return {};
}
render() {
return null;
}
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);
// Trigger sCU:
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});

it('disables logs once for class state updaters', () => {
spyOnDevAndProd(console, 'log');

let inst;
let count = 0;
class Foo extends React.Component {
state = {};
render() {
inst = this;
return null;
}
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);
inst.setState(() => {
count++;
console.log('foo ' + count);
return {};
});

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});

it('disables logs once for function double render', () => {
spyOnDevAndProd(console, 'log');

let count = 0;
function Foo() {
count++;
console.log('foo ' + count);
return null;
}

const container = document.createElement('div');
ReactDOM.render(
<React.StrictMode>
<Foo />
</React.StrictMode>,
container,
);

expect(count).toBe(__DEV__ ? 2 : 1);
expect(console.log).toBeCalledTimes(1);
// Note: we should display the first log because otherwise
// there is a risk of suppressing warnings when they happen,
// and on the next render they'd get deduplicated and ignored.
expect(console.log).toBeCalledWith('foo 1');
});
});
});