Better fix for base state bug#11273
Conversation
| }, | ||
| ); | ||
|
|
||
| it('uses correct base state for setState inside render phase', () => { |
There was a problem hiding this comment.
Before the fix, this would infinite loop. That's what was failing in www.
| ); | ||
|
|
||
| it('uses correct base state for setState inside render phase', () => { | ||
| spyOn(console, 'error'); |
There was a problem hiding this comment.
Would be nice to check for specific message/count. Just in case.
acdlite
force-pushed
the
fixbasestateagain
branch
from
c0a3b91 to
d03d6d3
Compare
| queue1 = fiber.updateQueue = createUpdateQueue(fiber.memoizedState); | ||
| // TODO: We don't know what the base state will be until we begin work. | ||
| // It depends on which fiber is the next current. Initialize with an empty | ||
| // base state, then set to the memoizedState when rendering. Not super |
There was a problem hiding this comment.
If we just remove the argument to createUpdateQueue would it be that bad?
There was a problem hiding this comment.
@gaearon I'd prefer to keep it for type safety for queues that don't belong to Fiber. Don't have any of those yet but could in the future (I do in one of my PRs).
| queue.expirationTime = NoWork; | ||
|
|
||
| let state = queue.baseState; | ||
| let state = queue.baseState === null |
There was a problem hiding this comment.
Perhaps comment should be moved here?
There was a problem hiding this comment.
I'll put it in both places :D
acdlite
force-pushed
the
fixbasestateagain
branch
from
d03d6d3 to
3bafdca
Compare
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
I don't see anything obviously wrong. Which could mean there's still issues but at least we have one more regression test now :-)
|
Ha indeed |
|
Does this affect corner cases like |
|
@gaearon Don't think so, we have tests for those so I would hope not. |
|
Maybe |
|
Could use a sentinel instead of |
|
Yeah, didn't want to add an extra field. I'll try to think of something better and if I can't I'll do that. |
|
I mean something like var NO_STATE_SENTINEL = {};
if (__DEV__) {
if (Object.freeze) {
Object.freeze(NO_STATE_SENTINEL);
}
}
let state = queue.baseState === NO_STATE_SENTINEL
? workInProgress.memoizedState
: queue.baseState;not sure if it makes sense |
3 participants
This time with a test. Not super happy with the solution, but I think it's good enough for now.