fix(setprops): allowed for setProps to be synced with nextTick intervals #1618

AtofStryker · 2020-07-22T03:15:00Z

setProps in certain cases was being blown away by nextTick intervals. If the property is not up to
date, setProps will be called again to sync the changes.

fix #1419

What kind of change does this PR introduce? (check at least one)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

It's submitted to the dev branch.
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup
New/updated tests are included

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

AtofStryker · 2020-07-22T03:20:30Z

packages/test-utils/src/wrapper.js

+        nextTick().then(() => {
+          const isUpdated = Object.keys(data).some(key => {
+            // $FlowIgnore : Problem with possibly null this.vm
+            return this.vm[key] === data[key]


not sure if we need to compare by value here, but figured this is handled above? Might also be a if the same object property is set, but another is not.

This is comparing by value right?

@lmiller1990 this would be a comparison by reference if we are considering objects/array. But these should be assigned by reference here which makes me think this should be enough to check the reference, unless something is getting mutated under the hood? Might be good to do a value check here to be safe

lmiller1990

Kinda hacky but many things are here :)

If you can get the rest of the tests to pass, I think we should be good.

lmiller1990 · 2020-07-22T07:33:13Z

packages/test-utils/src/wrapper.js

+        nextTick().then(() => {
+          const isUpdated = Object.keys(data).some(key => {
+            // $FlowIgnore : Problem with possibly null this.vm
+            return this.vm[key] === data[key]


This is comparing by value right?

AtofStryker · 2020-07-22T14:49:39Z

Kinda hacky but many things are here :)

If you can get the rest of the tests to pass, I think we should be good.

@lmiller1990 It looks like the tests are timing out? I was able to get them to pass locally so I am unsure of the issue

AtofStryker · 2020-07-22T15:26:49Z

test/specs/wrapper/setProps.spec.js

+      expect(callback.calledOnce)
+    })
+
+    it('invokes watchers with immediate set to "true" with deep objects', async () => {


also added a deep equality test here to test the assign by reference in the code to make sure we don't get into some type of continual recursive loop. This looks to work as intended

AtofStryker · 2020-07-22T15:27:31Z

Looks like this test is hanging?

lmiller1990 · 2020-07-22T22:09:40Z

I will take a look at the hanging test.

I think we should drop support for < Vue 2.5 sometime.

dobromir-hristov · 2020-07-29T05:56:54Z

Heh nice. I would not have thought about this :D

AtofStryker · 2020-07-30T00:09:42Z

@dobromir-hristov I almost wish I didn't think of this 😆 . It's pretty hacky, but wasn't sure of another way, especially since Watcher doesn't seem to do anything with the immediate option, except execute the callback 'immediately'

AtofStryker · 2020-07-30T00:10:11Z

@lmiller1990 any luck with that hanging test? Let me know if I can help in any way!

lmiller1990 · 2020-07-30T02:00:23Z

I spent a bit of time but I am not sure why is is hanging.

There is some way to skip tests for old Vue versions, you could give that a shot (if that's the reason it's hanging).

AtofStryker · 2020-07-30T02:17:50Z

I spent a bit of time but I am not sure why is is hanging.

There is some way to skip tests for old Vue versions, you could give that a shot (if that's the reason it's hanging).

@lmiller1990 is there a config val in CI that handles that? Otherwise, I could just use xdescribe on a commit for that particular test and push it up, if that is alright with you

lmiller1990 · 2020-07-30T09:26:58Z

@AtofStryker sorry, I mean you can use this helper:

vue-test-utils/test/specs/wrapper/trigger.spec.js

Line 112 in 1e3f7fb

itDoNotRunIf(

setProps in certain cases was being blown away by nextTick intervals. If the property is not up to date, setProps will be called again to sync the changes. fix vuejs#1419

AtofStryker · 2020-07-31T03:19:23Z

@lmiller1990 I figured out why the test was hanging. I completely forgot the $attrs use case, which was causing the test to go into an infinite recursion loop, which is bad 😬 . I have added an additional check here to compare for $attrs if the key doesn't exist on vm here. That should make the tests pass and not introduce breaking changes 😄

packages/test-utils/src/wrapper.js

Co-authored-by: Bill Glesias <bglesias@gmail.com>

lmiller1990 · 2020-08-01T13:12:26Z

Haha, so the test was actually correctly helping us out and catching the attrs edge case. Awesome! Great job on this, I will merge and do a release in the next week (probably with your babel dependencies ones that is merged).

99linesofcode · 2020-08-12T11:19:06Z

@AtofStryker if I may, did you happen to figure out why the next tick caused the setProps call to fail?

TeamDman · 2020-09-04T20:48:31Z

packages/test-utils/src/wrapper.js

+              this.vm[key] === data[key] || this.vm.$attrs[key] === data[key]
+            )
+          })
+          return !isUpdated ? this.setProps(data).then(resolve()) : resolve()


Shouldn't this be
.then(() => resolve())
instead of
.then(resolve())
?

EDIT: ignore this, I forgot that this was overriding the method, so we know it's always returning a promise
Maybe the typedef could be updated

Additionally, the signature on setProps claims to potentially return void

interface BaseWrapper { setProps (props: object): Promise<void> | void }

so I'm getting warnings on

? this.setProps(data).then(`

property 'then' does not exist on type 'void | Promise<void>'. Property 'then' does not exist on type 'void'

maybe I'm missing something, I'm just trying to backport the fix for my project

Janky backport function if anyone cares

/** * Bug: setProps doesn't play nice when there exists a watcher with immediate:true * https://github.com/vuejs/vue-test-utils/pull/1618 */ function setPropsForSure( wrapper: Wrapper<CombinedVueInstance<any, any, any, any, any>>, data: any, ) { return new Promise(resolve => { wrapper.setProps(data); wrapper.vm.$nextTick().then(() => { const isUpdated = Object.keys(data).every(key => { return ( // $FlowIgnore : Problem with possibly null this.vm wrapper.vm[key] === data[key] || wrapper.vm.$attrs[key] === data[key] ); }); if (isUpdated) { return resolve(); } let result = setPropsForSure(wrapper, data); if (result) { result.then(() => resolve()); } else { resolve(); } }); }); }

TeamDman · 2020-09-04T21:06:02Z

packages/test-utils/src/wrapper.js

-      return nextTick()
+      return new Promise(resolve => {
+        nextTick().then(() => {
+          const isUpdated = Object.keys(data).some(key => {


Shouldn't this use .every?
Otherwise, setting new props where one but not all of the new values is the same as the old could falsely signal that the update succeeded.

vegerot · 2020-09-14T20:07:25Z

Not sure if this is the culprit, but we have a test that appears to succeed, but never finishes. i.e the test reports success, but jest never exits. This was not a problem on 1.0.3 but is a problem on every version since.

Not a full repro, but the test is something like

    it('should use foo\'s dates when bar hasn\'t been set', () => {
      expect.assertions(2);
      wrapper.setProps({
        a: {
          ...a,
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
        b: {
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
      });
      expect(wrapper.vm.bar).toBeTruthy();

      wrapper.setProps({
        a: {
          ...a,
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
        b: {
          endTime: '2020-01-01',
          startTime: '2020-01-01',
        },
      });
      expect(wrapper.vm.bar).toBeFalsy();
    });

This test passes very quickly (about one second), but 10 minutes later and jest is still doing...something.

This is fixed by doing

    it('should use foo\'s dates when bar hasn\'t been set', async () => {
      expect.assertions(2);
      await wrapper.setProps({
        a: {
          ...a,
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
        b: {
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
      });
      expect(wrapper.vm.bar).toBeTruthy();

      await wrapper.setProps({
        a: {
          ...a,
          endTime: '2020-01-02',
          startTime: '2020-01-01',
        },
        b: {
          endTime: '2020-01-01',
          startTime: '2020-01-01',
        },
      });
      expect(wrapper.vm.bar).toBeFalsy();
    });

lmiller1990 · 2020-09-14T21:45:25Z

We should update the docs to encourage people to always do await setProps - would that solve the problem?

Actually instead of using setProps I just write two tests - I find that easier to debug anyway. But we should make sure setProps still works well

vegerot · 2020-09-17T18:08:39Z

Well for me, using await setProps solved my problem 🤷‍♀️

Serhansolo · 2020-09-25T16:22:46Z

@vegerot, I can't solve the same issue with awaiting. Does anyone know more about this weird issue and why it's happening?

AtofStryker commented Jul 22, 2020

View reviewed changes

lmiller1990 reviewed Jul 22, 2020

View reviewed changes

AtofStryker force-pushed the bugfix/setProps branch from ca56456 to 3b03990 Compare July 22, 2020 15:24

AtofStryker commented Jul 22, 2020

View reviewed changes

AtofStryker requested a review from lmiller1990 July 22, 2020 15:46

AtofStryker marked this pull request as ready for review July 22, 2020 16:14

fix(setprops): allowed for setProps to be synced with nextTick intervals

fa669fa

setProps in certain cases was being blown away by nextTick intervals. If the property is not up to date, setProps will be called again to sync the changes. fix vuejs#1419

AtofStryker force-pushed the bugfix/setProps branch from 3b03990 to fa669fa Compare July 31, 2020 03:02

AtofStryker commented Jul 31, 2020

View reviewed changes

packages/test-utils/src/wrapper.js Outdated Show resolved Hide resolved

AtofStryker mentioned this pull request Jul 31, 2020

Async watcher in some cases called twice after setProps. #1379

Open

Update packages/test-utils/src/wrapper.js

766fe00

Co-authored-by: Bill Glesias <bglesias@gmail.com>

lmiller1990 merged commit 9a3e6f9 into vuejs:dev Aug 1, 2020

AtofStryker mentioned this pull request Aug 9, 2020

Migrate to jest 2 #1629

Merged

13 tasks

xanf mentioned this pull request Aug 22, 2020

Change the way of setting props #1300

Closed

TeamDman reviewed Sep 4, 2020

View reviewed changes

xanf mentioned this pull request Dec 14, 2020

fix: prevent setProps infinite loop with immediate watchers #1752

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(setprops): allowed for setProps to be synced with nextTick intervals #1618

fix(setprops): allowed for setProps to be synced with nextTick intervals #1618

AtofStryker commented Jul 22, 2020

AtofStryker Jul 22, 2020

lmiller1990 Jul 22, 2020

AtofStryker Jul 22, 2020

lmiller1990 left a comment

lmiller1990 Jul 22, 2020

AtofStryker commented Jul 22, 2020

AtofStryker Jul 22, 2020

AtofStryker commented Jul 22, 2020

lmiller1990 commented Jul 22, 2020

dobromir-hristov commented Jul 29, 2020

AtofStryker commented Jul 30, 2020

AtofStryker commented Jul 30, 2020

lmiller1990 commented Jul 30, 2020

AtofStryker commented Jul 30, 2020

lmiller1990 commented Jul 30, 2020

AtofStryker commented Jul 31, 2020

lmiller1990 commented Aug 1, 2020

99linesofcode commented Aug 12, 2020

TeamDman Sep 4, 2020 •

edited

Loading

TeamDman Sep 4, 2020 •

edited

Loading

TeamDman Sep 4, 2020

vegerot commented Sep 14, 2020

lmiller1990 commented Sep 14, 2020

vegerot commented Sep 17, 2020

Serhansolo commented Sep 25, 2020

fix(setprops): allowed for setProps to be synced with nextTick intervals #1618

fix(setprops): allowed for setProps to be synced with nextTick intervals #1618

Conversation

AtofStryker commented Jul 22, 2020

AtofStryker Jul 22, 2020

Choose a reason for hiding this comment

lmiller1990 Jul 22, 2020

Choose a reason for hiding this comment

AtofStryker Jul 22, 2020

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Jul 22, 2020

Choose a reason for hiding this comment

AtofStryker commented Jul 22, 2020

AtofStryker Jul 22, 2020

Choose a reason for hiding this comment

AtofStryker commented Jul 22, 2020

lmiller1990 commented Jul 22, 2020

dobromir-hristov commented Jul 29, 2020

AtofStryker commented Jul 30, 2020

AtofStryker commented Jul 30, 2020

lmiller1990 commented Jul 30, 2020

AtofStryker commented Jul 30, 2020

lmiller1990 commented Jul 30, 2020

AtofStryker commented Jul 31, 2020

lmiller1990 commented Aug 1, 2020

99linesofcode commented Aug 12, 2020

TeamDman Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

TeamDman Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

TeamDman Sep 4, 2020

Choose a reason for hiding this comment

vegerot commented Sep 14, 2020

lmiller1990 commented Sep 14, 2020

vegerot commented Sep 17, 2020

Serhansolo commented Sep 25, 2020

TeamDman Sep 4, 2020 •

edited

Loading

TeamDman Sep 4, 2020 •

edited

Loading