Test renderer traversal

[sophiebits]

If you do

x = ReactTestRenderer.create(<a />);
x.update(<b />);
x.getType();

then you probably expect to get 'b' back. But if you were to do

div = ReactTestRenderer.create(<div><a /></div>);
x = div.getChildren()[0];
div.update(<div><b /></div>);
x.getType();

then since x points directly to the original <a /> child, there's no possible way we could return 'b'. So rather than have a confusing inconsistency between the top-level node and all the lower ones, @gaearon suggested outlawing changing type and key so that you always have the same instance.

I'm not actually sure I like this. It might make more sense to distinguish between the top-level instance and the children in the API -- since it also makes sense to call .update and .unmount at the top but nowhere else.

cc @sebmarkbage @rafeca #7148

[gaearon]

@cpojer @spicyj

Who plans to use this API?
Who is the right person to review this diff?

[cpojer]

This looks good to me.

Were you guys thinking of providing a .find(MyComponent) button inside of the test-renderer or should that be a separate package outside of React? I think this PR + find should cover most use cases people need right now.

[sophiebits]

I think we could add some sort of find APIs officially.

@sebmarkbage Do you feel good about the approach here of having .update() and .unmount() on every test instance but having them work only at the root (and outlawing key/type changes at the root)? Not sure if we should differentiate the so that the container/root is a different object from the rest.

[sebmarkbage]

I see. Yea, maybe separating them would be better even though it adds some mental overhead in the common case. This should probably not be the ideal end-user API anyway, maybe?

[sophiebits]

What should be the ideal end-user API? :)

[cpojer]

I think that it would be ideal to be able to do something like:

tree.find(Button).getProps().onClick()

that way people can drive interactions and make sure state changes can be captured in a snapshot. I think we should restrict search to non-host elements so that people don't write brittle tests, like making sure a span is at the right spot in the tree etc. This stuff should be captured in the snapshot.

[aweary]

Not sure if we should differentiate the so that the container/root is a different object from the rest.

It makes sense to just return an instance of ReactTestInstance in all cases, since we'll want update and unmount to exist and throw helpful errors instead of access errors when used improperly.

then since x points directly to the original child, there's no possible way we could return 'b'. So rather than have a confusing inconsistency between the top-level node and all the lower ones, @gaearon suggested outlawing changing type and key so that you always have the same instance.

Would it be enough to just document that traversal methods like getChildren return isolated instances that are not affected by updates to their root? That's how it works in Enzyme FWIW. The docs could just specify that you should run your query each time your root updates.

// DO: call the traversal method each time your root updates
var tree = ReactTestRenderer.create(<Foo bar="bar" />);
expect(tree.find(Button).getProps().bar).toEqual("bar");
tree.update(<Foo bar="qux"/>);
expect(tree.find(Button).getProps().bar).toEqual("qux"); // <- GOOD

// DONT: try to reference a child instance after an update has been
// made without updating the reference
var tree = ReactTestRenderer.create(<Foo bar="bar" />);
var button = tree.find(Button);
expect(button.getProps().bar).toEqual("bar");
tree.update(<Foo bar="qux"/>);
// button still refers to the button found on the outdated root
expect(button.getProps().bar).toEqual("qux"); // <- BAD
// would be fine if they did `button = tree.find(Button)` beforehand

Otherwise I think this is good and we should get the conflicts resolved and get this merged so we can work on the actual traversal API.

[sophiebits]

we'll want update and unmount to exist and throw helpful errors instead of access errors when used improperly

I'm not sure if this is a good argument. If you are using a type checker than a static type error is even better, which is only possible if they are different types.

[aweary]

Well, that's assuming you're using a static type system and that we're exporting the correct type definitions. As long as you also throw an error on that separate type for update and unmount, then I don't really see anything wrong with that approach.

[faceyspacey]

@spicyj what's the use case for .update()?

I understanding updating the props (if that was in fact a proposed feature) by why would you want to replace the whole component or its children? Shouldn't updating of children be the job of the component in how it responds to state and props changes? And should replacing the top level component be the job of a separate test?

What am I missing?

That said, it might be nice to have an updateProps() method so you can very easily test lifecycle handlers like componentWillReceiveProps and shouldComponentUpdate on the top level component instance.

[frantic]

Should this be instance._renderedChildren?

[sophiebits]

Yes.

[jquense]

👋 Just noting, that i've done a bunch of work on instance/element traversal for testing here: https://github.com/jquense/bill (heavily tested against the last 3 versions of react). Happy to offer any thoughts, or help to get better api's for this into react core.

[aweary]

I think this is unnecessary now that toTree has been added to the test renderer (#8931).

[sophiebits]

Hm.

[aweary]

All of the traversal methods defined in #7409 can be implemented using toTree, I've got a WIP demonstrating that. This also only touches the stack test renderer, and at this point it's probably not worth including.

What do you think?

[faceyspacey]

@aweary that looks awesome, I'm gonna give it a try. Can you explain more what you mean by:

This also only touches the stack test renderer, and at this point it's probably not worth including.

[sophiebits]

I haven't had a chance to really dig in, but it seemed like toTree might be lacking two things – ability to pull out just part of a tree rather than serializing the entire thing from scratch, and the ability to have persistent component identity and "hold onto" a reference to a component when updating. Do you know how that compares?

[aweary]

toTree recursively traverses from the root, and as far as I can tell there isn't a way to start that traversal at an arbitrary depth. What's the use case for that? For persistent identity, depends on what you want. You can store a reference to instance which is the stateNode. If stateNode is persistent across updates then I think that's possible? But as for the node object itself, it will be recreated every time toTree is called.

So toTree does solve traversal, but updates and persistent references may still need work. Either way, this PR isn't relevant anymore since it doesn't touch the fiber renderer, unless we want to reuse it for that work?

Can you explain more what you mean

@faceyspacey there's a new version of the test renderer ReactTestRendererFiber that uses the new Fiber renderer API. This is what will be used from version 16 and on. The renderer this PR works on is called the "stack", which is just the name for the old reconciler before Fiber.

[sophiebits]

This got merged in #10377.