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

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

Merged
merged 2 commits into from Aug 1, 2020

Conversation

AtofStryker
Copy link
Contributor

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)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

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:

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:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is comparing by value right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda hacky but many things are here :)

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is comparing by value right?

@AtofStryker
Copy link
Contributor Author

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

expect(callback.calledOnce)
})

it('invokes watchers with immediate set to "true" with deep objects', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Looks like this test is hanging?

@AtofStryker AtofStryker marked this pull request as ready for review July 22, 2020 16:14
@lmiller1990
Copy link
Member

I will take a look at the hanging test.

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

@dobromir-hristov
Copy link
Contributor

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

@AtofStryker
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@lmiller1990
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

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
Copy link
Contributor Author

@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 😄

Co-authored-by: Bill Glesias <bglesias@gmail.com>
@lmiller1990
Copy link
Member

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).

@lmiller1990 lmiller1990 merged commit 9a3e6f9 into vuejs:dev Aug 1, 2020
@AtofStryker AtofStryker mentioned this pull request Aug 9, 2020
13 tasks
@99linesofcode
Copy link
Contributor

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

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

@TeamDman TeamDman Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@TeamDman TeamDman Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
			}
		});
	});
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

vegerot commented Sep 14, 2020

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
Copy link
Member

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
Copy link

vegerot commented Sep 17, 2020

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

@Serhansolo
Copy link

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

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

Successfully merging this pull request may close these issues.

Watch with immediate totally breaks setProps
7 participants