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

Should update run all watchers? #283

Closed
eddyerburgh opened this issue Dec 20, 2017 · 14 comments · Fixed by #474
Closed

Should update run all watchers? #283

eddyerburgh opened this issue Dec 20, 2017 · 14 comments · Fixed by #474

Comments

@eddyerburgh
Copy link
Member

eddyerburgh commented Dec 20, 2017

At the moment, the update method re-renders the component.

I think it should also run all component and child component watchers, otherwise users will need to use Vue.nextTick in cases like this—https://github.com/lmiller1990/vue-test-utils/blob/309aebcad7c736cd926a469a7d724ea18fc0f54c/docs/en/guides/testing-async-components.md

I'm trying to think if this could cause unintended side effects. Making the change to update doesn't break any of the tests.

@lmiller1990
Copy link
Member

I think this would make sense, following the docs where it says "vue-test-utils runs synchronously". So a yes from me!

This may also be related to #176 ?

@Austio
Copy link
Contributor

Austio commented Dec 21, 2017

Agreed, update should update watchers

@Prior99
Copy link

Prior99 commented Feb 23, 2018

This is causing a lot of trouble in our project. What exactly is the reasoning of calling all watchers on every update? Even if the underlying values did not change? This is not reflected behaviour of a real-world Vue application. Is it possible to trigger an event without all watchers being called and influencing how the component behaves?

@blocka
Copy link

blocka commented Feb 23, 2018 via email

@Prior99
Copy link

Prior99 commented Feb 23, 2018

I think this should be reverted, or at least configurable. This especially makes it near impossible to test anything using https://github.com/sagalbot/vue-select

@Prior99
Copy link

Prior99 commented Feb 23, 2018

@blocka Perhaps one of us should open an issue for this?

@blocka
Copy link

blocka commented Feb 23, 2018 via email

@Mweydert
Copy link

I also agreed that there should be a way to run only watcher(s) with an underlying value that has changed.
In one of our project's component, a first watcher has an impact on the value watched by the 2nd watcher. Therefore running wrapper.update() do not allow to test the behaviour of the second watcher unitary.

As a workaround, wrapper.update() seems to be not necessary in order for the watcher to execute. Watchers are asynchronous so delay the assertion seems to be enough.

wrapper.vm.propertyWatched = value;
setTimeout(() => {
    // assert changes operated by watcher
}, 50);

@eddyerburgh eddyerburgh reopened this Feb 27, 2018
@Mweydert
Copy link

I came on an other side effect of update running all watchers...

I want to test that a click on a button update my component state. The component I am testing has child components that I want to stub for this test (using shallow).

Component :

/*
 * CustomButton.vue
 */
<template>
  <div>
    <button class="btn" @click="push">Hello !</button>
    <fake-component ref="fakeComponent" />
  </div>
</template>
<script>
import FakeComponent from './FakeComponent.vue'
export default {
  components: {
    FakeComponent
  },
  data() {
    return {
      buttonPushed: false,
      valueThatDoesntChange: false
    }
  },
  watch: {
    valueThatDoesntChange: {
      handler() {
        // Operate on my stubbed component
        this.$refs.fakeComponent.refresh()
      }
    }
  },
  methods: {
    push() {
      // Part of the component I am testing
      // Execute some stuff that has nothing to do with my stubbed component
      this.buttonPushed = true;
    }
  }
}
</script>

Test example:

import { shallow } from '@vue/test-utils';
import CustomButton from './CustomButton.vue';
describe("CustomButton component", () => {
  it("should set value of buttonPushed", () => {
    const wrapper = shallow(CustomButton);
    expect(wrapper.vm.buttonPushed).toBe(false);     
    wrapper.find(".btn").trigger("click");
    expect(wrapper.vm.buttonPushed).toBe(true);
  });
});

Tests are passing, but I get an error because the watcher try to execute a function of this stubbed component (that doesn't exist).

BTW thanks for this really useful library :-)

@Prior99
Copy link

Prior99 commented Mar 7, 2018

I can't express enough how problematic this is. I can't use trigger at all to trigger any events and instead have to resort to .element.dispatchEvent most of the times as triggering the watchers otherwise leads to unpredictable behavior not at all reflected in what would happen in real life.

@eddyerburgh
Copy link
Member Author

Yes I can see this is big problem we need to fix, I've been looking at potential solutions but haven't found one yet.

FYI update takes an optional object of values, and will only run wtacherrs that depend on that value:

wrapper.update({
  changedValue: true
})

I'm aware this doesn't solve the issue with trigger

@eddyerburgh
Copy link
Member Author

This should be fixed in #474.

This PR removes update entirely. Instead, it walks all watchers and makes them synchronous. This means everything (including re-renders) happens synchronously. This should solve your issues.

@Prior99
Copy link

Prior99 commented Apr 3, 2018

Does this avoid updating unrelated watchers when triggering?

@eddyerburgh
Copy link
Member Author

Yes it should do

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

Successfully merging a pull request may close this issue.

6 participants