Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow multiple root children in test renderer traversal API #13017

Merged
merged 1 commit into from
Jun 11, 2018
Merged

Allow multiple root children in test renderer traversal API #13017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 57 additions & 34 deletions packages/react-test-renderer/src/ReactTestRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,45 @@ const validWrapperTypes = new Set([
ClassComponent,
HostComponent,
ForwardRef,
// Normally skipped, but used when there's more than one root child.
HostRoot,
]);

function getChildren(parent: Fiber) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - we could explicitly flow type the return value; Array<ReactTestInstance | string>.

Also I see that this was copy-pasted from the getChildren method below. Wish there was a docblock, it's not very clear to me why this works the way it does. But that could be for the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't always Flow-type internal functions because those change more often. I figured that since we still have an outer annotation on the public API, we can leave this one inferred. If it returns something bad the annotation on the calling method should catch it.

const children = [];
const startingNode = parent;
let node: Fiber = startingNode;
if (node.child === null) {
return children;
}
node.child.return = node;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granted this is copy-pasted - but wondering if you know, why do we mutate the fiber instance to set it's return here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is a pattern we use everywhere we traverse fibers deeply. It's necessary because each fiber has two copies. The return pointer points to the parent but we don't know which copy (current of work in progress). If we just naïvely follow "return" to go up, we might end up in a different tree copy from the previous update.

This is why every time we descend down, no matter when, we always overwrite the .return pointer to the parent in the tree we're working on. This should have no observable side effect because we do this on every deep traversal, and we never trust the chain of return pointers to give us the "current" tree.

node = node.child;
outer: while (true) {
let descend = false;
if (validWrapperTypes.has(node.tag)) {
children.push(wrapFiber(node));
} else if (node.tag === HostText) {
children.push('' + node.memoizedProps);
} else {
descend = true;
}
if (descend && node.child !== null) {
node.child.return = node;
node = node.child;
continue;
}
while (node.sibling === null) {
if (node.return === startingNode) {
break outer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

20287394_1768381429856192_1934891266588278784_n

}
node = (node.return: any);
}
(node.sibling: any).return = node.return;
node = (node.sibling: any);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure it will become clear as I continue reading, but I was surprised that this method mutates the nodes and goes down to potentially get deeply nested children.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just copy pasted from below. I didn't write it.

It works this way to skip over fragments / context consumers / other wrappers in the tree, and only "see" custom components and host nodes in traversal APIs.

return children;
}

class ReactTestInstance {
_fiber: Fiber;

Expand Down Expand Up @@ -246,6 +283,13 @@ class ReactTestInstance {
let parent = this._fiber.return;
while (parent !== null) {
if (validWrapperTypes.has(parent.tag)) {
if (parent.tag === HostRoot) {
// Special case: we only "materialize" instances for roots
// if they have more than a single child. So we'll check that now.
if (getChildren(parent).length < 2) {
return null;
}
}
return wrapFiber(parent);
}
parent = parent.return;
Expand All @@ -254,38 +298,7 @@ class ReactTestInstance {
}

get children(): Array<ReactTestInstance | string> {
const children = [];
const startingNode = this._currentFiber();
let node: Fiber = startingNode;
if (node.child === null) {
return children;
}
node.child.return = node;
node = node.child;
outer: while (true) {
let descend = false;
if (validWrapperTypes.has(node.tag)) {
children.push(wrapFiber(node));
} else if (node.tag === HostText) {
children.push('' + node.memoizedProps);
} else {
descend = true;
}
if (descend && node.child !== null) {
node.child.return = node;
node = node.child;
continue;
}
while (node.sibling === null) {
if (node.return === startingNode) {
break outer;
}
node = (node.return: any);
}
(node.sibling: any).return = node.return;
node = (node.sibling: any);
}
return children;
return getChildren(this._currentFiber());
}

// Custom search functions
Expand Down Expand Up @@ -469,10 +482,20 @@ const ReactTestRendererFiber = {
configurable: true,
enumerable: true,
get: function() {
if (root === null || root.current.child === null) {
if (root === null) {
throw new Error("Can't access .root on unmounted test renderer");
}
const children = getChildren(root.current);
if (children.length === 0) {
throw new Error("Can't access .root on unmounted test renderer");
} else if (children.length === 1) {
// Normally, we skip the root and just give you the child.
return children[0];
} else {
// However, we give you the root if there's more than one root child.
// We could make this the behavior for all cases but it would be a breaking change.
return wrapFiber(root.current);
}
return wrapFiber(root.current.child);
},
}: Object),
);
Expand Down
44 changes: 44 additions & 0 deletions packages/react-test-renderer/src/__tests__/ReactTestRendererTraversal-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,48 @@ describe('ReactTestRendererTraversal', () => {
expect(nestedViews[1].parent).toBe(expectedParent);
expect(nestedViews[2].parent).toBe(expectedParent);
});

it('can have special nodes as roots', () => {
const FR = React.forwardRef(props => <section {...props} />);
expect(
ReactTestRenderer.create(
<FR>
<div />
<div />
</FR>,
).root.findAllByType('div').length,
).toBe(2);
expect(
ReactTestRenderer.create(
<React.Fragment>
<div />
<div />
</React.Fragment>,
).root.findAllByType('div').length,
).toBe(2);
expect(
ReactTestRenderer.create(
<React.Fragment key="foo">
<div />
<div />
</React.Fragment>,
).root.findAllByType('div').length,
).toBe(2);
expect(
ReactTestRenderer.create(
<React.StrictMode>
<div />
<div />
</React.StrictMode>,
).root.findAllByType('div').length,
).toBe(2);
expect(
ReactTestRenderer.create(
<Context.Provider>
<div />
<div />
</Context.Provider>,
).root.findAllByType('div').length,
).toBe(2);
});
});