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

Vue 3: about rerender api design #198

Open
cuixiaorui opened this issue Jan 8, 2021 · 10 comments
Open

Vue 3: about rerender api design #198

cuixiaorui opened this issue Jan 8, 2021 · 10 comments

Comments

@cuixiaorui
Copy link

Hi, I'm a little confused when using the rerender API

I wanted to update props, but then I realized that in the next release, I needed to call rerender api

However, I looked at the implementation of rerender and saw that the component instance was re-destroyed and then re-created

But my test requirement is to simulate that the user only updated props

For example, I did this earlier by calling the wrapper. setProps API for vue-test-utils-next

Now the new rerender behaves substantially differently than setProps

The former is destroyed to re-create components based on the new props
The latter is to change the props on a component that has been created previously

I need to have a consistent API and setProps behavior

@cuixiaorui cuixiaorui added the bug Something isn't working label Jan 8, 2021
@afontcu afontcu added vue3 and removed bug Something isn't working labels Jan 8, 2021
@afontcu
Copy link
Member

afontcu commented Jan 8, 2021

Hi! Thanks for your message.

Looks like you're right, I overlooked this bit: https://github.com/testing-library/vue-testing-library/pull/173/files#diff-25bf86bba97788343267cf1a217702b4825b65b411e7697b0daa7576a5eb79ecR23-R25

instance-id should be 1.

That being said:

I need to have a consistent API and setProps behavior

Moving from Vue 2 support to Vue 3 might yield different behaviors, so consistency is not assured. I understand you want a consistent API, but here we're trying to find the middle ground between vue test utils and testing library family.


Also, if you need for some reason to update props, it'd probably be better if you test the component that's doing the updating to ensure that the props are being updated correctly.

@cuixiaorui
Copy link
Author

cuixiaorui commented Jan 8, 2021

Thanks for your reply

I found an API for updateProps on the master version

updateprops is a nice API from a code intent point of view

What was the reason for its removal?

A reasonable design scenario for rerender might be:

Keep the previous Wrapper instance and only update it

Of course, from a code implementation point of view, it's much easier to destroy and then create

What do you think of the reasonable behavior of rereader?

@afontcu
Copy link
Member

afontcu commented Jan 8, 2021

Yep. I think we should:

  1. Go back to the original behavior of setProps/rerender.
  2. Find out if there's a way to keep the previous wrapper instance and update it, so it does not only update props, but any other information we pass down to the component.

That being said, (2) should not be that common because people should really just render the component again manually.

We changed names to align with other Testing Library solutions, which feature a rerender method.

@cuixiaorui
Copy link
Author

cuixiaorui commented Jan 8, 2021

thank you. I see. To keep the interface consistent

If rerender only has actions to modify props then we only need to call wrapper.setprops internally

As you said, the second point is not commonly used but is something to consider (this is a complication from rerender naming, people will use it to update other properties)

I don't know any other testing library render behavior is what looks like

@afontcu
Copy link
Member

afontcu commented Jan 8, 2021

@afontcu
Copy link
Member

afontcu commented Jan 8, 2021

(btw, I removed the bug label as now rerender what does it says on #176. However I leave the issue open to see if we find a better way to perform the rerendering operation)

@afontcu afontcu removed the bug Something isn't working label Jan 8, 2021
@afontcu afontcu mentioned this issue Jan 8, 2021
6 tasks
@afontcu afontcu changed the title about rerender api design VTL for Vue 3: about rerender api design Jan 8, 2021
@ITenthusiasm
Copy link
Contributor

ITenthusiasm commented Feb 10, 2021

Been so caught up with work that I missed this too. lol.

So for clarity (and people are free to disagree with me on this), I intentionally wrote the rerender function the way I did based on what I understood (or thought I understood) about React Testing Library and the other libraries in the TL family. To me, it wasn't simply renaming setProps to rerender, but it was actually causing a rerender itself. There are a few reasons for this:

  1. Looking at React Testing Library, rerender is synchronous. Going back to the hopes of unifying the libraries, it is rather weird to require people to await a rerender here but not there. I especially feel this way when VTL's render function is synchronous. Making rerender an alias for wrapper.setProps introduces asynchronicity again (unless that was changed).

  2. Continuing from the previous point, it's important to consider how people conceptually understand a "rerender". The word itself would lead one to conclude that something else is being rendered to the DOM. That's the behavior places likes RTL and STL follow, and that's why I implemented rerender the way I did. Personally, I feel that in a sense, supplying a means to setProps but calling it rerender is a bit misleading/dishonest (especially with the obvious need to do an await). The end result is the same, but I do feel there's a small difference.

  3. Again, looking at React Testing Library, it looked like rerender was supposed to overwrite the DOM. Originally, a previous component was there; now a new component is rendered to that location. (Admittedly, that implementation could just be due to the nature of how React works, rather than being intentional. I guess I can't say for sure. But STL does seem to take a similar approach.)

  4. This one is a bit subjective. lol. I get antsy when I see stuff like setProps because it starts to remind me of Enzyme. And I thought the TL family was intended to push away from that: to get us out of the idea of thinking of manipulating state and props, and to get us into the idea of interacting with a DOM. Although RTL lets you change props, if I understand correctly, you aren't working with the same instance of the component anymore. Yes, you lose access to the original component, but in my mind that again works towards moving people away from focusing on implementation details. Even in the Testing JavaScript series, Kent seems to suggest that rerender is only something you should use if you really need to.

I think (and I could be wrong) that some of the push back to the new API is because implementation from VTU leaked out into VTL. Instead of starting out with a literal rerender where the original DOM is replaced like in RTL/STL, VTL users were given direct access to wrapper.setProps. Though subtle, the behavior here is different, and people don't tend to favor change. My hope was that VTL would become more like the other libraries, and that people would be able to handle the adjustment since it's small. Needing to work with the original instance sounds more like an implementation detail, and in other libraries people are getting along just fine without that functionality.

I think if we're going to use rerender, then it should be a true rerender. Otherwise, a setProps function should be exposed. That of course creates inconsistency. But my concern with exposing rerender as an alias for setProps is that it'll create a way for VTL to diverge from the rest of the family without it being obvious that it's doing so. (Again, this becomes more obvious/concerning with needing to await rerender.) That's a more dangerous place to be in my opinion.

@afontcu
Copy link
Member

afontcu commented Feb 10, 2021

Thanks for getting back to this! I'll reread the whole thing to see what we can come up with 👍

However, re this

  1. Again, looking at React Testing Library, it looked like rerender was supposed to overwrite the DOM. Originally, a previous component was there; now a new component is rendered to that location. (Admittedly, that implementation could just be due to the nature of how React works, rather than being intentional. I guess I can't say for sure. But STL does seem to take a similar approach.)

This is the test that made me rollback changes in our rerender: https://testing-library.com/docs/example-update-props. Notice how the component instance doesn't change, so their definition of "rerendering" is that it updates the existing component instance rather than removing and mounting it again.

@ITenthusiasm
Copy link
Contributor

This is the test that made me rollback changes in our rerender: https://testing-library.com/docs/example-update-props. Notice how the component instance doesn't change, so their definition of "rerendering" is that it updates the existing component instance rather than removing and mounting it again.

Didn't see this. That's actually super helpful in understanding what's going on, so thanks a ton for pointing that out. I'll have to rethink how I think about rerender then. Maybe I don't fully grasp what the function is doing in RTL yet. More fun stuff to think about!

@afontcu afontcu changed the title VTL for Vue 3: about rerender api design Vue 3: about rerender api design Jul 26, 2021
@itutto
Copy link

itutto commented Oct 10, 2021

This discussion has been silent for a while, but in case there is still some debate about the future of the rerender function, i'd like add my my use case as an example why consistency across frameworks would be preferred:

I'm part of a team developing a design system and the same components are developed for multiple frameworks. I've been hoping that we could reuse the same tests for the same components with only the input component defined accordingly for each fw. The idea so far only broke when it came to the rerender statements within the tests.
Currently the only way I could get around the differences in implementation is to write internal wrapper functions that overwrite the return of the render function in way that provides both the RTL and VTL with a unified rerender behaviour.

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

No branches or pull requests

4 participants