Replace setProps with rerender Function
#173
Conversation
Codecov Report
@@ Coverage Diff @@
## next #173 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 53 58 +5
Branches 9 9
=========================================
+ Hits 53 58 +5
Continue to review full report at Codecov.
|
ITenthusiasm
force-pushed
the
add-rerender
branch
from
f435497 to
ba63200
Compare
|
Noticed that However, in the future, I think it would be worth simply pointing to the actual file. Doing so would remove the need for the special Jest configuration. More importantly, it would create less confusion for other contributors. Taking a quick look at RTL and STL, they seem to use the regular file name as well instead of using an alias. I'm not going to address that in this PR though. Just pointing something out so it's not forgotten. |
I did so to make it simpler for users to just "copy&paste" the examples into their codebase and get them working, but I'm totally open to suggestions :) |
src/vue-testing-library.js
Outdated
| @@ -47,7 +47,7 @@ function render( | |||
| // additionalOptions = configurationCb(router) | |||
| // } | |||
|
|
|||
| const wrapper = mount( | |||
| let wrapper = mount( | |||
There was a problem hiding this comment.
No need to make it mutable, right? Could we just create a local variable inside of rerender at leave this as a const?
There was a problem hiding this comment.
Yeah see, that's the hard part. Because the properties in the returned object point to wrapper, that binding is actually what needs to be updated. If wrapper is const, then it can't be reassigned. So when rerender is called, the original wrapper is destroyed, a new component is mounted, and the test looks fine. But any consecutive times that rerender is called in the same test (which is a realistic scenario), the code will be working with a reference to the oldest wrapper, not the new one. So nothing new will be destroyed, and the DOM will get polluted very quickly. Moreover, users will be unable to take advantage of all utility functions that use wrapper (for the aforementioned reason).
Theoretically, there might be some things we could try to keep it const? But in the interest of keeping things simple, I think it's best to use let. But as long as we're careful about how we use wrapper, we should be okay.
I promise I did try using const before my first commit, though! 😅
There was a problem hiding this comment.
Due to what I just described, I updated the first test to call rerender twice. This should help avoid unexpected bugs, like in the case of the updateProps test on master.
Since I didn't submit a code change for this one, I'll leave the resolution up to you. If you think of any simple workarounds let me know. I'll keep thinking while the PR is open.
src/vue-testing-library.js
Outdated
| wrapper = mount( | ||
| TestComponent, | ||
| merge({ | ||
| attachTo: container, | ||
| global: { | ||
| plugins, | ||
| }, | ||
| ...mountOptions, | ||
| props, | ||
| // ...additionalOptions, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Would it make sense to create come sort of a mountComponent function (or something with a better name 😇) to avoid repeating this code block? Both rendering methods must have the same keys, and having them in a single place looks like a good thing.
There was a problem hiding this comment.
Hm, it could even include the unwrap + add lines below 🤔
There was a problem hiding this comment.
React Testing Library has something like that. At a first glance I think it should be doable.
There was a problem hiding this comment.
I added my first iteration of the function. Let me know what you think! The 2 original blocks of code were slightly different due to the overwrite of props, so I had to include that in the arrow function. But besides that, I think it's okay. If you're fine with it, you can resolve this piece. If not, you can let me know! 😄
ITenthusiasm
force-pushed
the
add-rerender
branch
2 times, most recently
from
40eab96 to
9779bcc
Compare
* Removed `setProps` function (and tests) to confirm with Testing Library standards. * Added `rerender` function (and tests) to make the API more familiar to the Testing Library family.
ITenthusiasm
force-pushed
the
add-rerender
branch
from
9779bcc to
4dc3ef2
Compare
Ah, I see. That makes sense. I'll think about it a bit more before opening up a PR. Thanks! |
|
Hey! I noticed that you added some reactions to my comments that seemed positive. Since the PR hasn't been merged yet and I haven't seen any new comments, I just wanted to double check and make sure whether you wanted me to add any more changes or not. |
Hey! yeah, it looks good :) I'll be probably merging and releasing it soon. Thanks! 🚀 |
|
🎉 This PR is included in version 6.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
setPropsfunction (and tests) to confirm with TestingLibrary standards.
rerenderfunction (and tests) to make the API more familiar tothe Testing Library family.