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

Clarify DOM updates should be awaited on API documentation #1626

Closed
99linesofcode opened this issue Jul 29, 2020 · 3 comments
Closed

Clarify DOM updates should be awaited on API documentation #1626

99linesofcode opened this issue Jul 29, 2020 · 3 comments

Comments

@99linesofcode
Copy link
Contributor

What problem does this feature solve?

New developers such as me might not be aware that certain API methods are asynchronous and expect DOM updates to be handled automatically.

Specifically, the setProps() usage states: Sets Wrapper vm props and forces update. which threw me off.

Now, I realize this is blatantly obvious to those who know their way around Vue.js but I found myself spending quite a bit of time figuring this out.

What does the proposed API look like?

  1. We clearly state whether a method is performed asynchronously or is otherwise batched by Vue. ie. in the usage section.
  2. We link to the guide that explains this behavior in greater detail for all methods that can be awaited.
@99linesofcode
Copy link
Contributor Author

I will gladly make the necessary changes if we agree on a format. Let me know 👋

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 1, 2020

Yep... the await feature is pretty recent, actually - you had to do await wrapper.vm.$nextTick prior to this. now you can just do await wrapper.setProps.

I don't know of a specific format, I think we should go through and update everything to encourage using await for basically anything that might trigger an update. So we should say something like "any time you call a method that will change the rendered HTML, you should use await.

Maybe we can just recommend any method with set in the name, or trigger should use await 🤔

What format/update are you thinking @99linesofcode ? Can you give an example how you think we can make it more clear?

@afontcu probably has some input here!

@99linesofcode
Copy link
Contributor Author

99linesofcode commented Aug 12, 2020

Ok, so my current project is still on version 1.0.0-beta.32 and using await wrapper.setProps(data) worked inside an async test. To be honest, I'm unsure as to why wrapping wrapper.setProps() in a Promise actually works in the example below. If anyone could shine some light on that, much appreciated. But I digress..

    it('renders an error message when the error property is passed', async () => {
        expect(wrapper.find('span').text()).toEqual(wrapper.vm.error);

        await wrapper.setProps({ error: null });

        expect(wrapper.find('span').exists()).toBe(false);
    });

Yep... the await feature is pretty recent, actually - you had to do await wrapper.vm.$nextTick prior to this. now you can just do await wrapper.setProps.

Yeah, #1517 introduced this change. I see the API docs for .trigger() were updated. Sounds like a good idea to make similar changes to the API docs for all the other setter methods.

What format/update are you thinking @99linesofcode ? Can you give an example how you think we can make it more clear?

I think simply updating the examples as you proposed should suffice. I would also include a See also: Testing Asynchronous Behavior link to the guide.

Let me make the necessary changes and tell me whether I missed something 😄

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

2 participants