-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
Added memoization test for interrupted low-priority renders #8645
Added memoization test for interrupted low-priority renders #8645
Conversation
FYI @gaearon, this is the failing test we were discussing on PR #8631. The same failure highlighted in this test applies to Do you think it would be useful for me to expand the test to include a check for |
Hmm maybe I misunderstand but I thought this was intentional. You get "last attempted" as prev in sCU but "last rendered" as prev in cDU. Is this a problem in some scenario? |
9ee0624
to
382fd04
Compare
Hmm. It's possible it is intentional and I just misunderstood the intent. That would make sense too as a behavior. I suppose I just assumed symmetry of before and after values between those methods (cWRP/sCU/cWU/cDU).
I'm not sure people think of Either way, I guess you're right. I don't think it would cause problems unless users set some sort of expectations in cWRP/sCU/cWU that were checked in cDU which seems unlikely. Looking at the different ways in which Maybe @sebmarkbage can clarify or point out a case I'm not thinking about. We talked about this briefly last week in the context of a larger discussion about If this is indeed the expected behavior then perhaps I could modify my test slightly (so that it passes) and capture it as such. |
Thought about this for a bit. The mental model for To put this in more practical terms, if a high priority update interrupts a low priority update before it is flushed, we should have the ability to reuse the children computed during the low priority update. Which we can't do if we reset the pointers. I don't think we have a test for this, but we should. Care to write one? :) |
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.
Change test to check that a high-priority update can reuse the children from an aborted low-priority update using shouldComponentUpdate
.
I agree that this is unlikely. A better name for |
^ The test I describe above happens to be related to a PR I was already working on, so I added it there. https://github.com/facebook/react/pull/8655/files#diff-6a068060b8f49a9d61ae16cac3be4a9b |
Wouldn't that essentially be the test I've added, if I only changed an expectation or two (and maybe added a render counter)? That's why I left the previous comment:
I'll update the test in this PR. |
382fd04
to
d814ef5
Compare
Thanks for the input Dan and Andrew. I've updated this PR to capture what I think has been agreed on as the expected/correct behavior (a new passing test instead of a failing one). |
// Renders: Root | ||
ReactNoop.render(<Root>B</Root>); | ||
ReactNoop.flushDeferredPri(20); | ||
|
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.
Should you be asserting that what you expected happened? Otherwise you can merge the call below into this one.
expect(scuPrevProps.children).toEqual('A'); | ||
expect(scuNextProps.children).toEqual('B'); | ||
expect(cduPrevProps).toEqual(null); | ||
expect(cduNextProps).toEqual(null); |
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.
These could have been called with null if there was a bug. I tend to prefer pushing to an array since that covers that and tests execution order.
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.
Good suggestion.
d814ef5
to
0e954f9
Compare
0e954f9
to
e95aaa7
Compare
@@ -2184,4 +2184,87 @@ describe('ReactIncremental', () => { | |||
'componentDidUpdate', | |||
]); | |||
}); | |||
|
|||
it('should reuse memoized work after an interrupted low-priority render', () => { |
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.
Let's rename the test so it's clear that this is specifically about updating the pointers before calling lifecycles.
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.
👍
Test added to verify that a high-priority update can reuse the children from an aborted low-priority update if shouldComponentUpdate returns false.
e95aaa7
to
2fb07b9
Compare
Added a test to verify that a high-priority update can reuse the children from an aborted low-priority update if
shouldComponentUpdate
returns false.(Note that this PR is now different from the original. The original description is preserved below so that the subsequent discussion on this PR makes sense.)
Previous Description (no longer valid)
this.props
andthis.context
are not reset when a render is interrupted mid-way and overridden by a subsequent, higher-priority update. The result is that a subsequent calls toshouldComponentUpdate
will yield unexpected results1 when comparingthis.props
andthis.context
to the providednexProps
andnexContext
.This PR captures the behavior in a (failing) unit test. Posting for discussion purposes.