[Fiber] Nesting validation warnings #8586

gaearon · 2016-12-16T00:39:27Z

Dirty right now, will clean up tomorrow. But it almost works.

Not very happy with different host context types in DEV and PROD but I don't know how else to avoid extra allocations in PROD.

It is annoying to have to recalculate own context for non-reified text in host components so I probably need to pass own context to hosts. Also I might need to add something like getRootHostContext to avoid passing root instance to every getChildContext.

sebmarkbage · 2016-12-16T08:41:39Z

I don't think we should under estimate how dangerous the complexity of DEV only things can be if we're not careful. They can sometimes make it difficult to see how code could be simplified because they add a lot of noise.

We should also be careful about exposing too much to host config. I only exposed the internal instance for the event system because we really needed it for event system compatibility but I think we should be very hesitant to expose more internals or even make any more use of the internal instance handle.

That said we can probably find a simple way to implement it.

This particular warning is also not really that relevant outside server rendering.

gaearon · 2016-12-16T10:43:01Z

IMO it's useful on client side (for example I often put a tags into a tags by mistake and this screws up the browser).

But I don't think I fully understand why the logic for the message is so complex. I'll see if I can just use the element stack instead.

sophiebits

For the owner stack we can probably just do the full parent stack (using the standard helper) – that includes host components right?

sophiebits · 2016-12-16T22:14:29Z

src/renderers/dom/fiber/ReactDOMFiber.js

+      if (hostContext) {
+        validateDOMNesting(type, null, internalInstanceHandle, hostContext.ancestorInfo,);
+      }
+      if (typeof props.children === 'string' || typeof props.children === 'number') {


Can this just live in createTextInstance?

No because this is the "single child" case in which we don't create an instance but use setTextContent.

gaearon · 2016-12-16T22:24:06Z

For the owner stack we can probably just do the full parent stack (using the standard helper) – that includes host components right?

Yea, doing exactly that now, seems to work well (haven't pushed yet).

sebmarkbage · 2016-12-16T23:50:27Z

@gaearon It only screws up the browser if you put invalid tags in innerHTML, not if you add them manually. So when React switched to document.createElement it should no longer be a big deal, even if you use Stack, other than if you use server-rendering.

gaearon · 2016-12-16T23:58:48Z

I already figured and was very surprised. 😄
Still, we'd need warning parity so that people who roll back from Fiber don't have a ton of warnings.

sebmarkbage · 2016-12-17T00:02:26Z

We could disable it in Stack too when useCreateElement is true. That will still trigger it during server-rendering.

I'm not sure we should because now it's harder to move to server rendering after the fact but I'm just saying it's plausible.

gaearon · 2016-12-17T00:05:50Z

Okay, pushed a new version.

It doesn't require passing Fibers to renderer, and uses the element stack.
If this is an okay direction I can fix up Flow and polish this.

Still not happy about DEV branches (and Flow hacks we'd have to add for them) but I can add some more production tests for this to make sure we don't regress.

WDYT?

gaearon · 2016-12-17T00:07:19Z

(If this approach is cool I can also make Stack use element stack here too.)

gaearon · 2016-12-17T00:10:59Z

src/renderers/dom/fiber/ReactDOMFiber.js

-  getChildHostContext(parentHostContext : string | null, type : string) {
-    const parentNamespace = parentHostContext;
-    return getChildNamespace(parentNamespace, type);
+  getRootHostContext(rootContainerInstance) {


One benefit of this is that we should be able to mount SVG content into an svg root now.
Can add tests for this.

gaearon · 2016-12-17T00:11:57Z

src/renderers/dom/fiber/ReactDOMFiber.js

  ) : boolean {
+    if (__DEV__) {
+      if (oldProps.children !== newProps.children && (


I should probably check that we're moving to having a non-reified child here instead of always running the check.

Also ^^^ the day I started saying "reified". What have you done to me.

gaearon · 2016-12-17T00:15:26Z

src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js


 function flattenChildren(children) {
  var content = '';

  // Flatten children and warn if they aren't strings or numbers;
  // invalid types are ignored.
+  // We can silently skip them because invalid DOM nesting warning
+  // catches these cases in Fiber.


It was a bit easier to keep nesting checks in ReactDOMFiber than shoving them inside ReactDOMFiberComponent. As a result I'm dealing with rawProps rather than "host props" and validate them earlier than in Stack. I could have fixed this but it felt like the nesting check is already generic enough that we can just remove this special case. I could also remove it in Stack if needed.

sebmarkbage · 2016-12-17T00:15:46Z

src/renderers/dom/fiber/ReactDOMFiber.js

+      const isMountingIntoDocument = rootContainerInstance.ownerDocument.documentElement === rootContainerInstance;
+      const ancestorInfo = getChildAncestorInfo(null, isMountingIntoDocument ? '#document' : type);
+      return {namespace, ancestorInfo};
+    } else {


Nit: We don't really use else with DEV blocks because it didn't use to work in our transforms but I actually prefer that pattern because it enforces the notion that __DEV__ is only additional and doesn't change behavior. For this case I tend to prefer to use an early return instead. Although in this case I guess it severely changes the whole return type.

gaearon · 2016-12-17T00:16:11Z

src/renderers/dom/shared/validateDOMNesting.js

@@ -316,6 +318,49 @@ if (__DEV__) {
    return stack;
  };

+  var getOwnerInfo = function(childInstance, childTag, ancestorInstance, ancestorTag, isParent) {


This is the Stack-specific part extracted from main function.
In Fiber instances are null and we use the current element stack instead.

gaearon · 2016-12-17T00:20:04Z

src/renderers/art/ReactARTFiber.js

@@ -485,6 +485,10 @@ const ARTRenderer = ReactFiberReconciler({
    // Noop
  },

+  getRootHostContext() {
+    return null;


IIRC you wanted something like this anyway.

sebmarkbage · 2016-12-17T00:21:18Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

@@ -731,4 +733,20 @@ var ReactDOMFiberComponent = {

 };

+if (__DEV__) {
+  ReactDOMFiberComponent.getChildAncestorInfo = function(parentAncestorInfo, type) {


I don't like these forwarding things. We have so much of these in the event system that just adds bloat, add risk for cyclic dependencies and confusion.

Do you have a plan with these or can you just move this to ReactDOMFiber?

Btw, the only reason for a separate ReactDOMFiberComponent file is to keep parity with Stack so that we can easier replicate changes. Should get rid of this file later.

Right. I thought I'd need them inside but then turned out I didn't. Will remove.

gaearon · 2016-12-17T00:21:22Z

src/renderers/shared/fiber/ReactFiberHostContext.js

  } = config;

  // Context stack is reused across the subtrees.
  // We use a null sentinel on the fiber stack to separate them.
-  let contextFibers : Array<Fiber | null> | null = null;
-  let contextValues : Array<CX | null> | null = null;
+  let contextFibers : Array<Fiber | null> = [];


Because we now always use this for the root instance.

sebmarkbage · 2016-12-17T00:35:33Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    if (__DEV__) {
+      const namespace = getChildNamespace(null, type);
+      const isMountingIntoDocument = rootContainerInstance.ownerDocument.documentElement === rootContainerInstance;
+      const ancestorInfo = getChildAncestorInfo(null, isMountingIntoDocument ? '#document' : type);


Does the child nesting validation not case about the namespace?

Good catch. It doesn't (in Stack neither), and this is a bug. Because that should be valid: https://jsfiddle.net/q7e61jpd/.

sebmarkbage

Ok, some nits but this strategy seems fine.

This makes it easier to avoid accidental nulls. I also added a test for the production case to avoid regressing because of __DEV__ branches.

gaearon · 2016-12-17T02:43:36Z

The last few commits are ready for final review.

gaearon · 2016-12-17T02:58:21Z

I'll just get this in and address issues in a follow up (if any).

sebmarkbage · 2016-12-20T02:27:43Z

src/renderers/dom/fiber/ReactDOMFiber.js

+      const ancestorInfo = updatedAncestorInfo(null, isMountingIntoDocument ? '#document' : type, null);
+      return {namespace, ancestorInfo};
+    }
+    return getChildNamespace(null, type);


@gaearon This doesn't seem right. If I'm rendering SVG into an SVG tree, then the current namespace isn't HTML and the tag name isn't enough to determine it since I could be rendering into any SVG root. It seems to me that we should be reading the namespace of the root instead of the tagName.

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 16, 2016

sophiebits reviewed Dec 16, 2016

View reviewed changes

gaearon force-pushed the warnings-3 branch from de3de51 to 6e3e009 Compare December 17, 2016 00:03

gaearon force-pushed the warnings-3 branch from 6e3e009 to 1ea4e1e Compare December 17, 2016 00:06

(WIP) Nesting warnings

d36ba98

gaearon force-pushed the warnings-3 branch from 1ea4e1e to d36ba98 Compare December 17, 2016 00:10

gaearon commented Dec 17, 2016

View reviewed changes

sebmarkbage reviewed Dec 17, 2016

View reviewed changes

gaearon commented Dec 17, 2016

View reviewed changes

sebmarkbage reviewed Dec 17, 2016

View reviewed changes

gaearon commented Dec 17, 2016

View reviewed changes

sebmarkbage reviewed Dec 17, 2016

View reviewed changes

sebmarkbage approved these changes Dec 17, 2016

View reviewed changes

gaearon added 3 commits December 17, 2016 01:49

Remove indirection

fd036bd

Add a note about namespace

997fe3a

Fix Flow and make host context required

414f1ec

This makes it easier to avoid accidental nulls. I also added a test for the production case to avoid regressing because of __DEV__ branches.

gaearon changed the title ~~[Fiber][WIP] Nesting validation warnings~~ [Fiber] Nesting validation warnings Dec 17, 2016

gaearon merged commit 70f704d into facebook:master Dec 17, 2016

gaearon deleted the warnings-3 branch December 17, 2016 02:58

sebmarkbage reviewed Dec 20, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Nesting validation warnings #8586

[Fiber] Nesting validation warnings #8586

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 16, 2016

gaearon commented Dec 16, 2016

sophiebits left a comment

sophiebits Dec 16, 2016

gaearon Dec 16, 2016

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 16, 2016

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 17, 2016 •

edited

Loading

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

gaearon Dec 17, 2016

gaearon Dec 17, 2016

gaearon Dec 17, 2016

gaearon Dec 17, 2016

sebmarkbage Dec 17, 2016

gaearon Dec 17, 2016

gaearon Dec 17, 2016

sebmarkbage Dec 17, 2016

sebmarkbage Dec 17, 2016

gaearon Dec 17, 2016

gaearon Dec 17, 2016

sebmarkbage Dec 17, 2016 •

edited

Loading

gaearon Dec 17, 2016

sebmarkbage left a comment

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

sebmarkbage Dec 20, 2016

gaearon Dec 25, 2016

[Fiber] Nesting validation warnings #8586

[Fiber] Nesting validation warnings #8586

Conversation

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 16, 2016

gaearon commented Dec 16, 2016

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 16, 2016

gaearon commented Dec 16, 2016

sebmarkbage commented Dec 17, 2016 • edited Loading

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Dec 17, 2016

gaearon commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Dec 17, 2016 •

edited

Loading

sebmarkbage Dec 17, 2016 •

edited

Loading