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.

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

Shallow Renderer should expose rendered component instance #4056

Closed
jsdf opened this issue Jun 8, 2015 · 7 comments
Closed

Shallow Renderer should expose rendered component instance #4056

jsdf opened this issue Jun 8, 2015 · 7 comments

Comments

@jsdf
Copy link
Contributor

jsdf commented Jun 8, 2015

Just as ReactMount.render returns the rendered component instance, it seems to me that ReactShallowRenderer.prototype.render should return the rendered component or expose it in some other way (maybe a getRenderedComponent method) so that when testing stateful components via shallow rendering, assertions can be made against the state of the component instance.

@jsdf
Copy link
Contributor Author

jsdf commented Jun 8, 2015

A hacky workaround is to access the internals of the ReactShallowRenderer but this seems like something which should be part of the public API.

@sebmarkbage
Copy link
Collaborator

Yea, I agree that it is probably useful as a convenience similar to refs. Since a ref could expose a public API which can be tested. It is probably bad practice to call handlers or inspect state directly though. It is better to test the public API by invoking the callbacks of the render output.

@hungtuchen
Copy link

It is probably bad practice to call handlers or inspect state directly though

@sebmarkbage
how if we can state on text input or seletedIndex on input that would set on state,
and they are later to submit when ex: a button being clicked,
however they are intentionally being set and related to rendered output.
In this case, I can' think of a better way other than inspecting the state if we want to test

@glenjamin
Copy link
Contributor

At the moment there's no way to test things like componentDidMount or componentDidUpdate via shallow rendering, exposing the instance would provide a way to do this.

@glenjamin
Copy link
Contributor

I've put together a side-by-side comparison of lifecycle support between "normal" rendering and "shallow" rendering.

Exposing getMountedInstance() would allow any lifecycle methods to be called, but perhaps it would be a better fix to make the shallow rendering engine call the other methods?

http://jsfiddle.net/xvs6xdfk/3/

Output using normal rendering

Rendering Comp with Object {key: 1, a: 1}
componentWillMount []
render []
componentDidMount []
Rendering Comp with Object {key: 1, a: 2}
componentWillReceiveProps [Object, Object]
shouldComponentUpdate [Object, null, Object]
componentWillUpdate [Object, null, Object]
render []
componentDidUpdate [Object, null, Object]
Rendering Comp with Object {key: 2}
componentWillUnmount []
componentWillMount []
render []
componentDidMount []

Output using shallow rendering

Rendering Comp with Object {key: 1, a: 1}
componentWillMount []
render []
Rendering Comp with Object {key: 1, a: 2}
componentWillReceiveProps [Object, Object]
shouldComponentUpdate [Object, null, Object]
componentWillUpdate [Object, null, Object]
render []
Rendering Comp with Object {key: 2}
componentWillReceiveProps [Object, Object]
shouldComponentUpdate [Object, null, Object]
componentWillUpdate [Object, null, Object]
render []

We're heavy shallow rendering users on the current project at work - we're using https://github.com/glenjamin/skin-deep to provide some helper functions - including adding getMountedInstance to allow us to test the full lifecycle scenarios.

We should be able to contribute a PR for this if we get some steer on the best way to proceed.

@jsdf
Copy link
Contributor Author

jsdf commented Sep 18, 2015

Is it worth creating a new issue specifically for 'ensuring shallow rendered components can be progressed through the full lifecycle' eg. not leaving out componentDidMount and componentDidUpdate?

@sebmarkbage
Copy link
Collaborator

@jsdf Yea, that seems reasonable so that they can be addressed separately. Both seems reasonably simple though so a pull request would be welcome.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants