Feedback needed: add option to enable effects and lifecycle methods in shallow renderer #15589
Conversation
| this._validateCurrentlyRenderingComponent(); | ||
| this._createWorkInProgressHook(); | ||
|
|
||
| //fill in impl |
There was a problem hiding this comment.
This is the part I'm trying to still figure out how to do. I think I have a general idea of how things work, but before I went any further I wanted to confirm that this makes sense.
| }; | ||
|
|
||
| constructor() { | ||
| constructor(useLifecycleMethods = false) { |
There was a problem hiding this comment.
The way this option is passed can be changed. I just did this for now because it was easy.
Details of bundled changes.Comparing: c7398f3...5245fe7 react-test-renderer
Generated by 🚫 dangerJS |
|
Are there any eyes on this? The approach here seems reasonable, and Enzyme seems to be blocked on this (or something like it) getting in. |
|
@gaearon we discussed this in this enzyme issue. can you point me to the right person to give feedback on this please? I think this would allow a lot of people to start using hooks that are currently blocked from it because they use shallow rendering in tests. |
|
Wouldn't this always just be a poor man's version of enzyme's |
|
@audiolion isn't that true for all shallow rendering? I'm just proposing calling the functions in useEffect instead of doing nothing. We already simulate the behavior of other hooks, so useEffect seems appropriate to do too. And I'm suggesting exposing the behavior behind an option in the shallow renderer to avoid a breaking change. Also, just to be clear, this is not to enable testing of custom hooks (though I suppose it could help with that too). It's to enable testing of components that call useEffect (or some other hook that calls useEffect). |
|
Just want to chime in and say it would be great to be able to test components that have |
|
Is there a way to get this PR looked at? It's been open for a while now? |
|
Would love to see a maintainer put some eyes on this. Would make the hooks testing story with libraries like enzyme much better. |
|
Another bump on getting some maintainers to take a look at this change. |
|
Hey has anyone looked at this yet? |
🙏 📿
|
@acdlite Sorry to ping you on it, but it does not seem to have any sort of response, seems to be a relatively simple change, and is blocking pretty much anyone using shallow render from using hooks. |
|
Hate to keep bumping this but I think it's preventing a lot of people from using |
| // because DOM refs are not available. | ||
| if(this._useLifecycleMethods) { |
There was a problem hiding this comment.
I'm still trying to get feedback on the implementation plan. I'll clean up spacing and things like that at the end
|
|
||
| // Force user to opt in to calling componentDidUpdate() and | ||
| // getSnapshotBeforeUpdate because DOM refs are not available. | ||
| if(this._useLifecycleMethods) { |
|
I wrote a complete version of this work here: #16168 It might be a good idea to close this PR so we can concentrate on the complete fix and avoid spreading the gaze of the maintainers too thinly? |
|
closing this in favor of #16168 |
This PR is not complete yet, and once I've confirmed I'm on the right track I can close it and reopen it when it's actually finished if that is preferred. But I wanted to make sure this general approach is going to work.
This addresses issue #15275 by adding an option in the shallow renderer to run componentDidUpdate, componentDidMount, useEffect, and useLayoutEffect. This will allow libraries like enzyme to support running lifecycle methods when shallow rendering in components that have hooks. They already support this in class components by running the lifecycle instance methods directly, but that won't work for useEffect, and it seems like the support for that should live in the shallow renderer.