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] Plan forward #17321

Open
gaearon opened this issue Nov 8, 2019 · 35 comments · May be fixed by #18144
Open

[Shallow Renderer] Plan forward #17321

gaearon opened this issue Nov 8, 2019 · 35 comments · May be fixed by #18144

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Nov 8, 2019

Let's discuss what to do with the shallow renderer here. As I mentioned in #16168 (comment), we aren't using it much and don't consider it a best practice. So we aren't going to be very good stewards of its API going forward.

My proposal was for Enzyme or folks interested in it to copy the code into another repo, and continue maintaining it there under a difference package name. It would make sense to Enzyme to start depending on that fork. We would then deprecate react-test-renderer/shallow in favor of the community-maintained fork.

If you'd like to volunteer to set up a repo, please let us know in this issue!

cc @davidmarkclements regarding the proposed react-shallow-renderer package name.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Nov 8, 2019

The main concern I have isn’t maintenance of the code - it’s how it might break with new react releases.

Given that most of what the dev tools does seems to overlap with what enzyme needs, i wonder if a better path might not be in sharing those components between enzyme and the dev tools?

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 8, 2019

At least for shallow renderer, I wouldn't expect breakage with new React releases because the code itself is standalone. It can lag behind in features but it doesn't actually depend on React internals the way the more invasive introspection does. This is because it's completely implemented from scratch and doesn't use the real codepaths.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 8, 2019

I'm not sure I see how DevTools would be helpful given that DevTools introspect a real tree — but shallow renderer is a 100% custom implementation that doesn't use any of the real reconciliation mechanisms.

@kumar303

This comment has been minimized.

Copy link

@kumar303 kumar303 commented Nov 8, 2019

This comment about shallow renderer's dependencies implies that forking it into a standalone module would require copying/pasting a lot of core code.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 8, 2019

This is not a lot of code. Each of these is a tiny utility and they probably don't add up to more than ~200 lines.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 8, 2019

Also you can replace invariant with throw Error, warning with console.error, and objectIs with Object.is.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Nov 8, 2019

That's a fair point; it would still be preferable to avoid it lagging behind in new features if possible.

@tomstuart

This comment has been minimized.

Copy link

@tomstuart tomstuart commented Nov 9, 2019

For clarification: what is the main reason that the regular test renderer can’t support a shallow mode where composite child components are left inline instead of recursively rendered into host components? Is it for reasons of API backward compatibility, a fundamental limitation of the test renderer, a design principle, or something else?

@robertknight

This comment has been minimized.

Copy link
Contributor

@robertknight robertknight commented Nov 9, 2019

If it wasn't for the need to support the very large number of tests that I presume already exist, I think I would advocate for Enzyme to deprecate (and maybe even eventually drop) its shallow rendering mode entirely and instead document methods for users to mock at the import level as Dan described here. Anecdotally I observed that the differences in normal vs shallow rendering in Enzyme were the source of quite a lot of confusion for other devs on my team and that led us to replace it with normal rendering + import-level mocking instead.

If Enzyme were to do that, perhaps the shallow renderer could be "frozen" so that it is kept working, but no new features are added to it as a matter of policy.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Nov 10, 2019

@robertknight That presumes that it's actually a better pattern to do explicit import mocking of each sub-component (it also presumes that a pattern that forces jest is a reasonable thing to recommend). This isn't the place for a longer writeup of why shallow rendering is critical, a good pattern, and required for robust testing, however.

@robertknight

This comment has been minimized.

Copy link
Contributor

@robertknight robertknight commented Nov 10, 2019

That presumes that it's actually a better pattern to do explicit import mocking of each sub-component (it also presumes that a pattern that forces jest is a reasonable thing to recommend).

I agree that both of those things would be undesirable. We automatically mock imported components using some simple heuristics rather than requiring developers to remember to configure each one. We also use a Babel plugin, which is not tied to any particular test runner, rather than Jest, to do this.

This isn't the place for a longer writeup of why shallow rendering is critical, a good pattern, and required for robust testing, however.

That's fair enough. Perhaps I can continue the discussion in an issue on the Enzyme repo?

@Sonic12040

This comment has been minimized.

Copy link

@Sonic12040 Sonic12040 commented Nov 11, 2019

Would you be willing to speak more about the automatic mocking? I'd love to learn about the flow that you've found successful and the use-cases.

Edit: Realize I didn't tag @robertknight

@thedanchez

This comment has been minimized.

Copy link

@thedanchez thedanchez commented Nov 19, 2019

@gaearon @ljharb I'd like to volunteer setting up a repo if no else will -- appreciate any direction on the details of it. But I think it's important to keep the ball moving on this endeavor and not letting it go stale.

@thedanchez

This comment has been minimized.

Copy link

@thedanchez thedanchez commented Dec 2, 2019

@gaearon @ljharb Just wanted to follow up on my volunteering around this plan forward.

@thedanchez

This comment has been minimized.

Copy link

@thedanchez thedanchez commented Dec 27, 2019

@gaearon @ljharb Any updates on this? I'm still waiting, willing and trying to help here.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Dec 27, 2019

I remain happy to take over the shallow renderer, but the work to maintain it would be prohibitive unless the react team was willing to commit to helping to keep its tests passing as a precondition for any React releases (whether submitting failing test cases or notifying about upcoming failed tests with sufficient advance notice that i can make fixes, or whether making the fixes themselves)

@perrin4869

This comment has been minimized.

Copy link

@perrin4869 perrin4869 commented Dec 30, 2019

Does this mean the shallow renderer is practically abandoned?

@JimLynchCodes

This comment has been minimized.

Copy link

@JimLynchCodes JimLynchCodes commented Jan 7, 2020

@gaearon Myself and some of my colleague engineers are using react-test-renderer/shallow and do consider it to be a best practice. Unit testing is about "testing at the edges", and philosophically a parent component should only care that its children are rendered with the correct props without caring about the children components' implementation details, and this is exactly what shallow rendering is. Do you disagree with this?

@thedanchez

This comment has been minimized.

Copy link

@thedanchez thedanchez commented Jan 28, 2020

@gaearon I have to say I'm very discouraged by the lack of activity on what I think to be a very important topic of discussion here. When will the React Core Team revisit this?

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Jan 29, 2020

@thedanchez

As far as I'm aware, there is nothing that prevents you from creating a repository, moving the code there, and releasing a package. Shallow renderer has no dependencies on the actual React implementation, so you shouldn't need our permission or assistance to start working on this.

I'm sorry I haven't replied to you earlier — as you can see, there are 476 other open issues, and notifications occasionally get lost. I wasn't actively checking this issue because, again, there is nothing from our side that prevents you from starting on this.

Let me know if there's something else you want us to revisit!

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Jan 29, 2020

cc @NMinhNguyen who also expressed interest in setting up the repository and a package. He has a good understanding of how these are set up in general, so he can help you out if you're blocked on figuring this out.

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Jan 29, 2020

@gaearon I’ll start a new repo tomorrow (ish)

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 29, 2020

@gaearon it's not just about setting up a package tho; is there any interest from the react team in #17321 (comment) ?

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Jan 29, 2020

@ljharb #16168 (comment)

We'd be happy to run some tests against a separate package fwiw. Similar to how we have integration tests with create-react-class.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 29, 2020

Specifically, this part:

commit to helping to keep its tests passing as a precondition for any React releases

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 29, 2020

With that commitment, I'll make a repo for it this week, and start moving towards using it in enzyme.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Jan 29, 2020

I'm not sure I understand what this means. How would a React release be able to break the shallow renderer if the shallow renderer doesn't depend on React internals?

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Jan 29, 2020

Does this sound reasonable? We already publish @next builds so you can use them in the integration tests. If something breaks, you can let us know here. https://reactjs.org/blog/2019/10/22/react-release-channels.html#using-the-next-channel-for-integration-testing

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Jan 29, 2020

FYI I've begun work on extracting the shallow renderer and got all tests passing locally: https://github.com/NMinhNguyen/react-shallow-renderer. Will work on setting up builds and CI etc. later this week

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 29, 2020

@gaearon by changing what "rendering" means such that the shallow renderer's output no longer mirrors the React's output (for the first level, ofc).

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Feb 2, 2020

Hey @davidmarkclements, I've now set up a new repo for react-shallow-renderer, would you be able to transfer the npm package to my username, minh.nguyen?

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Feb 3, 2020

@NMinhNguyen if you're interested in making it work for enzyme, please add me on github and npm as well.

@robertknight

This comment has been minimized.

Copy link
Contributor

@robertknight robertknight commented Feb 10, 2020

I mentioned in an earlier comment that we replaced shallow rendering with a different approach that provides the same benefits, using semi-automatic mocking with a Babel plugin. I found some time to write up a blog post with details and links to real-world examples in a reasonably complex application. We are using Preact, but the approach works exactly the same way with React. I hope this is of use to anyone who is evaluating whether to continue using the shallow renderer or do something else.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Feb 10, 2020

This is great. Want to add a link to this plugin to our testing page where we mention module mocking?

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Feb 26, 2020

@davidmarkclements has transferred the package name to me and I've just published the initial version of it, v16.12.0 - https://www.npmjs.com/package/react-shallow-renderer. @ljharb I've added you on both npm and GitHub.

NMinhNguyen added a commit to NMinhNguyen/react that referenced this issue Feb 27, 2020
@NMinhNguyen NMinhNguyen linked a pull request that will close this issue Feb 27, 2020
NMinhNguyen added a commit to NMinhNguyen/react that referenced this issue Feb 27, 2020
NMinhNguyen added a commit to NMinhNguyen/react that referenced this issue Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.