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: prevent setProps infinite loop with immediate watchers #1752

Merged
merged 1 commit into from Jan 5, 2021

Conversation

xanf
Copy link
Contributor

@xanf xanf commented Dec 14, 2020

#1618 introduced a straightforward approach in attempt to synchronize props when immediate watchers are available.
This approach has several significant caveats:

  • it makes setProps behavior inconsistent - it will be either sync or async depending on your luck and component under test
  • it creates weird infinite loops sometimes as reported even when used with await

This pull request includes two tests which fail with current implementation and addresses this problem in multiple ways:

  • basically it is revival of Change the way of setting props #1300 done in a bit more clear way
  • setProps now will throw if used on non-root instances. I've tested this behavior (both specs of this project and gitlab specs does not have this behavior) and it is extremely inconsistent and error-prone. I think forbidding such operations will lead to better tests anyway
  • remove useless silent option from config. This option was used only for suppressing setProps warnings

Side-effects of this code change which I really like:

  • setProps now goes through props validation. I've caught several failing tests in GitLab when we were setting for example number where we declaring prop as string type
  • setProps now always needs to be await-ed. I like this consistency and how it aligns with setData, trigger, etc.

Basically people just should add async to any setProps calls in their code, which could be easily code-moded

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

The PR fulfills these requirements:

* prevent `setProps` from being called on non-top level wrappers
* remove useless `silent` option from config
@xanf xanf changed the title fix: prevent infinite loop with setProps with immediate watchers fix: prevent setProps infinite loop with immediate watchers Dec 14, 2020
@lmiller1990
Copy link
Member

Sorry - I must have missed the notification for this one. I will get this reviewed and merged in the next week. setProps is so complex!

@lmiller1990 lmiller1990 self-requested a review December 31, 2020 12:18
@afontcu
Copy link
Member

afontcu commented Jan 1, 2021

Hey! 👋 The PR looks great and I love the side effects it provides, but since it introduces a breaking change and 2.X is reserved to VTU for Vue 3, what's the release strategy, here?

@@ -8,7 +8,9 @@

Sets `Wrapper` `vm` props and forces update.

**Note the Wrapper must contain a Vue instance.**
::: warning
`setProps` could be called only for top-level component, mounted by `mount` or `shallowMount`
Copy link
Member

Choose a reason for hiding this comment

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

not 100% sure but looks like a typo? ignore me if I'm wrong

Suggested change
`setProps` could be called only for top-level component, mounted by `mount` or `shallowMount`
`setProps` should be called only for top-level component, mounted by `mount` or `shallowMount`

@lmiller1990
Copy link
Member

Right, I missed the "setProps now will throw if used on non-root instances" line. I wonder how common this is?

I share @afontcu's concerns - does a breaking change like this make sense at this point in the library's life cycle? I think this change is good and the code looks great!

That said I believe this is the same behavior in VTU 2.x ... so maybe it makes sense to introduce it here, to help people prepare for migration 🤔

I think we should see what the behavior of 2.x is first and see if it lines up with this breaking change. I can look into this.

If we decide to merge this, how do we handle the version? We can't do a 2.x since 2.x is used for Vue 3's version.

An alternative would be a feature flag - if we decide this will be the correct behavior and make 2.x align with it, we could hide this behind a flag and let people enable it when they are ready to migrate (or disable it if they are not, which is the default, I am not sure, I guess it would enabled by default?)

What do you all think? I like this change and the fact we are setting a kind of spec/rules for how VTU works.

@xanf
Copy link
Contributor Author

xanf commented Jan 4, 2021

@lmiller1990 @afontcu I would consider this as just 1.2. My position:

  • setProps on child components was extremely flaky already. Any top-level rerender killed that setProps operation result, and if you had watchers attached... it was killed 95% of times, so while formally this is change, because this was never documented, from my (biased) perspective, it's more like bugfixing
  • I've checked gitlab and bootstrap-vue codebase - none used setProps on child component. And considering we have all kind of different codesmells in gitlab codebase - I would like to make wild guess that impact of this change will be extremely low.

What we could do is in changelog mention something like: "setProps on child components is considered invalid. You could do Object.assign(wrapper.find(SomeYourComponent).vm, newProps) if you rely on this behavior but be wary this is (and was) fragile and error-prone"

Alternatively I could add current, let's call it "legacy" strategy for setting props on child components. I'd prefer not to, since this is definitely strong antipattern, but I could do if we have a consensus here

@afontcu
Copy link
Member

afontcu commented Jan 4, 2021

Yeah, kind of agree with all of you here. An alternative would be to display a console.error (or similar) instead of throwing, so that tests wouldn't change their behavior, thus avoiding a breaking change.

That being said, I'm OK with releasing 1.2 too, given the nature of the change and the versioning limitation we're facing 👍🏼

@xanf
Copy link
Contributor Author

xanf commented Jan 4, 2021

@afontcu giving console.error unfortunately won't work - my current approach simply impossible to apply to child component (it relies on top-level wrapper generated by VTU)

@lmiller1990
Copy link
Member

Ok let's do it, 1.2 sounds good to me. I will make sure to reference this PR and the discussion for others to follow.

I agree that setProps on a child is pretty unusual and not common in the various OSS code bases I have seen.

Thanks @xanf! I will release this and VTU v2 in the next day or two.

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.

None yet

3 participants