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

Feedback needed: add option to enable effects and lifecycle methods in shallow renderer #15589

Closed
wants to merge 1 commit into from

Conversation

bdwain
Copy link

@bdwain bdwain commented May 8, 2019

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.

this._validateCurrentlyRenderingComponent();
this._createWorkInProgressHook();

//fill in impl
Copy link
Author

Choose a reason for hiding this comment

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

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) {
Copy link
Author

Choose a reason for hiding this comment

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

The way this option is passed can be changed. I just did this for now because it was easy.

@sizebot
Copy link

sizebot commented May 8, 2019

Details of bundled changes.

Comparing: c7398f3...5245fe7

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js 0.0% -0.0% 508.77 KB 508.77 KB 108.96 KB 108.96 KB UMD_DEV
react-test-renderer.production.min.js 0.0% -0.0% 61.99 KB 61.99 KB 19.05 KB 19.05 KB UMD_PROD
react-test-renderer.development.js 0.0% -0.0% 504.31 KB 504.31 KB 107.86 KB 107.86 KB NODE_DEV
react-test-renderer.production.min.js 0.0% -0.0% 61.68 KB 61.68 KB 18.9 KB 18.9 KB NODE_PROD
react-test-renderer-shallow.development.js +2.5% +1.5% 41.49 KB 42.52 KB 10.64 KB 10.8 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+5.1% 🔺+2.7% 11.54 KB 12.13 KB 3.54 KB 3.64 KB UMD_PROD
react-test-renderer-shallow.development.js +2.9% +1.6% 35.72 KB 36.75 KB 9.3 KB 9.44 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+5.0% 🔺+2.6% 11.73 KB 12.31 KB 3.66 KB 3.76 KB NODE_PROD
ReactShallowRenderer-dev.js +3.0% +1.8% 34.68 KB 35.72 KB 8.65 KB 8.81 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@louisscruz
Copy link

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.

@bdwain
Copy link
Author

bdwain commented May 28, 2019

@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.

@audiolion
Copy link

Wouldn't this always just be a poor man's version of enzyme's mount that could potentially have bugs where behavior is different? I know other libraries that use this approach create a dummy component and insert the hook(s) into it and run the full lifecycle to test them.

@bdwain
Copy link
Author

bdwain commented May 28, 2019

@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).

@Joonpark13
Copy link

Just want to chime in and say it would be great to be able to test components that have useEffect with shallow in enzyme. Would love to get someone from the react team's eyes on this!

@bdwain
Copy link
Author

bdwain commented Jun 21, 2019

Is there a way to get this PR looked at? It's been open for a while now?

@agrasley
Copy link

Would love to see a maintainer put some eyes on this. Would make the hooks testing story with libraries like enzyme much better.

@hweeks
Copy link

hweeks commented Jun 24, 2019

Another bump on getting some maintainers to take a look at this change.

@Vetrano89
Copy link

Hey has anyone looked at this yet? :godmode: 🙏 📿

@Alphy11
Copy link

Alphy11 commented Jul 11, 2019

@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.

@Joonpark13
Copy link

Hate to keep bumping this but I think it's preventing a lot of people from using useEffect and shallow. Would it be possible to at least get some kind of indication that this will be looked at in the future? Or if this is not something that the team is willing to consider, then a comment saying so, so we can find a workaround?

// because DOM refs are not available.
if(this._useLifecycleMethods) {

Choose a reason for hiding this comment

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

spacing

Copy link
Author

Choose a reason for hiding this comment

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

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) {

Choose a reason for hiding this comment

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

spacing

@insidewhy
Copy link

insidewhy commented Jul 21, 2019

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?

@Joonpark13
Copy link

I wrote a complete version of this work here: #1616

Thanks for this! You meant to say #16168 though, I think?

@bdwain
Copy link
Author

bdwain commented Jul 25, 2019

closing this in favor of #16168

@bdwain bdwain closed this Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.