-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
[Fiber] Fix some of the warnings #8570
Conversation
When an element changes, we should copy the source and owner again. Otherwise they can get stale since we're not reading them from the element.
Fixes an accidental omission when both source and owner are null but displayName exists.
When we're in Fiber mode we don't actually expect that warning being printed.
e8960e3
to
fbde80c
Compare
var owner = fiber._debugOwner; | ||
var source = fiber._debugSource; | ||
if (owner == null && source == null) { | ||
return ''; |
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.
Even if we don't have owner and source, we still want the name of this fiber, right?
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.
Yes, a later commit fixes this.
} | ||
return fiber; | ||
|
||
return (fiber : any); // Don't set dev-only fields in production |
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.
If you annotate the variable declaration as Fiber, can you remove this cast?
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.
I actually didn't need it anymore because I annotated fields themselves as optional. Fixed.
var node = fiber; | ||
do { | ||
info += describeFiber(node); | ||
node = node.return; |
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.
It seems like this .return
can be wrong (can be the parent's alternate rather than the parent). The type will never differ and the owner rarely will, but the source could. You can imagine something like this:
if (!isFancy) {
return <div>{props.children}</div>;
}
return <div className={...} ref={...} style={...}>{props.children}</div>;
In that case, it would be best if we could show the correct source. Perhaps you could use ReactFiberTreeReflection to find the correct one. If you're not going to tackle this now, can you at least add a TODO?
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.
It is possible for a single Fiber to have been rendered by two different parents. E.g. in your example the props.children
may have bailed out when the parent switched. So at any given point it can be both.
I think it may just depend on context. In some context you may want the "current" Fiber path but in the case of the key warning you probably want the path you took to get to the update which will have the correct .return
pointers in that case.
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.
If it is meant to only be used during reconciliation and not at random times or commit phase, then we should name it to reflect that. We could even enforce it by making it getStackAddendumByCurrentFiber()
reading the currently executing Fiber.
But I'm not sure what you're plan is for things outside of reconciliation since these example are all during reconciliation as far as I can tell.
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.
My tentative plan is to figure out a way to get the tree hook working but I want to get the most important warnings (propTypes and keys) working by simpler means first. It gives me a better understanding of the problem to solve these separately (and maybe unify later) than try to unify right away.
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.
Okay, I'm just using ReactCurrentOwner
for this now. Seems to work well so far.
'renderer. It is likely to have bugs, breaking changes and is ' + | ||
'unsupported.' | ||
); | ||
if (!ReactDOMFeatureFlags.useFiber) { |
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.
Thanks, I knew this failed in Fiber but didn't think of doing this.
if ( | ||
knownKeysInDev != null && | ||
newChild != null && | ||
typeof newChild.key === 'string' |
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.
This should also have the $$typeof check. The logic here feels pretty brittle.
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.
Yea, is there anyway you can just avoid adding it by doing the check below this condition instead?
@@ -564,6 +613,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { | |||
let resultingFirstChild : ?Fiber = null; | |||
let previousNewFiber : ?Fiber = null; | |||
|
|||
let knownKeysInDev = 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.
This whole validation is going to be pretty expensive with all these extra temporary allocations anyway. I would suggest that we just do two loops over the array or iterable so that the whole logic can be isolated to a separate function and not bloat the already complex diffing which is about to be even more complex with further optimizations.
It might even be faster due to better memory locality.
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.
Thank you for explaining. I fixed this in 5724657.
2105300
to
1577957
Compare
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.
I don't think we should use ReactCurrentOwner for this. We can use a different module/field that does the same thing in DEV only but not the production one. Because this has runtime semantic meaning and we want to be able to control where it is used.
ReactCurrentOwner is also effectively deprecated in React so it's confusing that we add more uses of it when we want to minimize them.
@@ -53,7 +38,7 @@ function checkReactTypeSpec( | |||
location: ReactPropTypeLocations, | |||
componentName, | |||
element, | |||
debugID, | |||
stack, |
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.
Oops, this is not a good change. It would be expensive to always calculate it.
return getComponentName(owner); | ||
} | ||
// In Fiber, current owner could be any fiber. | ||
let fiber = ((owner : any) : Fiber); |
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.
Owner could also be a Stack instance. #8458
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.
Hmm, didn't earlier test exclude Stack owner? if (typeof owner.tag !== 'number') {
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.
Oh you're right, I missed it.
container | ||
); | ||
}).toThrowError( | ||
'This DOM node was rendered by `Owner`.' |
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.
In the future we should change this warning to include the stack.
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.
Yep. Aiming for parity for now but we should really revisit them all.
We keep known keys in a set in development. There is an annoying special case where we know we'll check it again because we break out of the loop early. One test in the tree hook regresses to the failing state because it checks that the tree hook works without a Set available, but we started using Set in this code. It is not essential and we can clean this up later when we decide how to deal with polyfills.
It needs to be available both to Fiber and Isomorphic because the tree hook lives in Isomorphic but pretty-prints Fiber stack.
The goal is to use ReactCurrentOwner less and rely on ReactDebugCurrentFiber for warning owner name and stack.
Fiber used a helper so two tests had the same phrasing. Stack also used a helper for most invariants but hardcoded a different phrase in one place. I changed that invariant message to use a helper which made it consistent with what it prints in Fiber.
1577957
to
0aeda52
Compare
Rebased incorporating the feedback. Stopped using |
1b72eb2
to
caed41b
Compare
This is ready for review. See 6796336 as the commit in the middle that changed since last review. The rest is the same until two new commits (05bf2e3 and caed41b). The remaining warnings are mostly SSR and tree hook (only used in Perf as I used return pointer and mutable global for other use cases). There's DOM nesting validation and a few composite warnings missing which I'll try to add tomorrow but I'd like to do it after landing this. |
@@ -1436,7 +1436,7 @@ describe('ReactDOMComponent', () => { | |||
|
|||
it('should warn about class', () => { | |||
spyOn(console, 'error'); | |||
ReactDOMServer.renderToString(React.createElement('div', {class: 'muffins'})); | |||
ReactTestUtils.renderIntoDocument(React.createElement('div', {class: 'muffins'})); |
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.
I suppose we should ideally have all our warnings replicated on the server and therefore have test for each one in a server-environment. Unless it's for updates etc.
@@ -101,6 +102,9 @@ var DOMRenderer = ReactFiberReconciler({ | |||
props : Props, | |||
rootContainerInstance : Container, | |||
) : void { | |||
if (__DEV__) { | |||
validateProperties(domElement, type, null, props); |
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.
Why don't we just set ReactDebugCurrentFiber
during the commit phase too so that it's easy for any renderer to get to this debug information during whatever validation they want?
This looks fine to me except 0aeda52 It would be nicer to always have the current debug Fiber available to renderers so that they can do the validation whenever is most convenient rather than having to split it up and do multiple passes. |
This gets mount-time CSS warnings to be printed. However update-time warnings are currently ignored because current fiber is not yet available during the commit phase. We also regress on HostOperation hook tests but this doesn't matter because it's only used by ReactPerf and it doesn't work with Fiber yet anyway. We'll have to think more about it later.
This makes it available during updates, fixing the last failing test in CSSPropertyOperations.
It is not clear if the old hook system is worth it in its generic incarnation. For now I am just hooking it up to the DOMFiber renderer directly.
This helps us track which warnings are really failing in Fiber, and which ones depend on SSR.
caed41b
to
837e683
Compare
I think everything is addressed now. Waiting for build to pass and then merging. |
Since we don't know if fibers will be committed or not we don't want to use the component tree. Instead we use the
return
pointers to form the stack since we have them in Fiber.I'm keeping
debugID
because we'll likely want it for other warnings / perf / DevTools but this part doesn't rely on it. Instead I added_debugSource
and_debugOwner
that get copied from the element.