[Fiber] Component lifecycle tests #8949
Conversation
scripts/fiber/tests-failing.txt
Outdated
| src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js | ||
| * should carry through each of the phases of setup | ||
| src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js | ||
| * should batch unmounts |
There was a problem hiding this comment.
Oops just noticed this
acdlite
force-pushed
the
fibercomponentlifecycletests
branch
2 times, most recently
from
7e26a90 to
38ecbc1
Compare
| if (typeof instance.componentWillUnmount === 'function') { | ||
| safelyCallComponentWillUnmount(current, instance); | ||
| } | ||
| ReactInstanceMap.remove(instance); |
There was a problem hiding this comment.
Never safe to rely on unmount.
| }).not.toThrow(); | ||
|
|
||
| // Stack doesn't warn | ||
| if (ReactDOMFeatureFlags.useFiber) { |
There was a problem hiding this comment.
Why?
Also, if it doesn't warn, do you think this could create warning noise in codebases that already rely on this pattern? It would be nice to check Power Editor and other React-heavy projects for this.
There was a problem hiding this comment.
My hunch is that setState on an unmounting parent in componentWillUnmount isn't that common but yeah I'll check
There was a problem hiding this comment.
Code changed, moot now anyway
acdlite
force-pushed
the
fibercomponentlifecycletests
branch
from
38ecbc1 to
617e7d3
Compare
|
@sebmarkbage Updated |
Need to reset current owner to null before committing
|
|
||
| module.exports = function( | ||
| scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void, | ||
| scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel, callerName : string | null) => void, |
There was a problem hiding this comment.
I'm not a fan of this pattern since they don't get stripped in prod. Can we reword it to be setState, replaceState or forceUpdate in the message instead of complicating the code?
There was a problem hiding this comment.
I'm passing null in prod. But yeah, I like that better anyway.
There was a problem hiding this comment.
Should do the same in ReactFiberUpdateQueue https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberUpdateQueue.js#L217
acdlite
force-pushed
the
fibercomponentlifecycletests
branch
2 times, most recently
from
426acd9 to
c725ed4
Compare
|
@sebmarkbage Updated again |
|
Forgot to update the Stack warnings |
acdlite
force-pushed
the
fibercomponentlifecycletests
branch
from
c725ed4 to
e9aca8e
Compare
|
Fixed |
We want to avoid passing around caller names because stripping them out in production complicates the code for little benefit.
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
Don't forget to update the whitelists!
| 'componentDidUpdate instead.', | ||
| getComponentName(owner) || 'A component' | ||
| ); | ||
| (owner: any)._warnedAboutRefsInRender = true; |
There was a problem hiding this comment.
Oops. Should we preventExtensions on fibers? Like we do in Stack instantiate.
There was a problem hiding this comment.
Yea. Let's add it in the constructor in DEV.
There was a problem hiding this comment.
But expandos on the class isn't really better neither.
5 participants
Fixes tests in ReactComponentLifeCycle-test.js
Biggest change is to set current owner to null at beginning of commit phase.