[Fiber] Mount/unmount warnings and invariants #8919
Conversation
scripts/fiber/tests-failing.txt
Outdated
| @@ -42,6 +40,17 @@ src/renderers/shared/__tests__/ReactPerf-test.js | |||
| * should work when measurement starts during reconciliation | |||
|
|
|||
| src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js | |||
| * ignores null children | |||
There was a problem hiding this comment.
Why have they moved to failed?
There was a problem hiding this comment.
Just noticed that, not sure why
acdlite
force-pushed
the
fibermounttests
branch
from
787baab to
f09f632
Compare
|
@gaearon Ok fixed it |
| } | ||
|
|
||
| if (__DEV__) { | ||
| const isRootRenderedBySomeReact = Boolean(container._reactRootContainer); |
There was a problem hiding this comment.
Can you verify the logic here? This means if you call render on a container with existing React children created by another React, we don't warn? I guess that is consistent with before.
There was a problem hiding this comment.
Yeah we currently only warn about different copies of React on unmount
| @@ -673,6 +673,7 @@ describe('ReactUpdates', () => { | |||
| it('should flush updates in the correct order across roots', () => { | |||
| var instances = []; | |||
| var updates = []; | |||
| var container = document.createElement('div'); | |||
There was a problem hiding this comment.
Hm. This looks wrong. This changes the behavior we're testing. Why doesn't it work?
| @@ -681,20 +682,21 @@ describe('ReactUpdates', () => { | |||
| } | |||
|
|
|||
| componentDidMount() { | |||
| this.container = document.createElement('div'); | |||
There was a problem hiding this comment.
Specifically we want to test that we can render into the node we already rendered.
There was a problem hiding this comment.
It does work the old way but it warns. Stack doesn't warn but it seems like it should?
Why doesn't Stack warn?
There was a problem hiding this comment.
It's allowed to render into an empty element. This test intentionally tests that so your change messes that up.
Only warn if the container has a React-rendered child.
acdlite
force-pushed
the
fibermounttests
branch
from
f09f632 to
9fcf761
Compare
|
@spicyj Fixed |
| ' This may be caused by unintentionally loading two independent ' + | ||
| 'copies of React.' : | ||
| '' | ||
| ) |
There was a problem hiding this comment.
Do these strings get compressed? Would it be better if we split them into separate invariants?
There was a problem hiding this comment.
It would be nice if error code transform was throwing on anything other than static messages.
There was a problem hiding this comment.
@gaearon: It does; this is a %s param so you can put whatever you want there.
There was a problem hiding this comment.
Yeah, I copy and pasted this from Stack. I'll change it in both places to use multiple invariants instead.
That way the messages are extracted by the error code transform.
Fixes some of the tests in
ReactMount-test.js, except the ones related to server-side rendering.