Skip to content

Deep watchers create infinite recursion #60

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

Closed
dougalg opened this issue Nov 6, 2019 · 8 comments
Closed

Deep watchers create infinite recursion #60

dougalg opened this issue Nov 6, 2019 · 8 comments
Assignees
Labels

Comments

@dougalg
Copy link

dougalg commented Nov 6, 2019

When using the shim, if you have a deep watcher on a prop that is defined as a VueTypes.* it creates an infinite loop. This is because Vue attempts to look up isRequired which is a getter that returns this. If deep: true is set on the watcher, then it attempts to look at the value returned from isRequired which is the parent object, so it recurses infinitely.

I think the simplest way to resolve this bug is, unfortunately, a breaking change. Change .isRequired to .isRequired(). This way it will return a function, rather than this when it is accessed by Vue.

@dwightjack
Copy link
Owner

Hi,

If everything works fine with the real library. The error could be related to the fact that the shim does not define isRequired as non-enumerable and then it can be listed by methods like Object.keys. The reason for that is to keep the source of the shims as small as possible.

I'm not quite sure about your use case though. Are you setting the value of a prop to a VueType.* type? Could you provide an example implementation?

@dougalg
Copy link
Author

dougalg commented Nov 7, 2019

Thanks for the response. I've been trying to get a minimal repro working, but it's actually not happening. I had the same question as you, about setting the value of a prop to a VueType, but it looks like for some reason, Vue is actually deep watching the entire grandparent component object definition (including it's non-evaluated prop definitions). I'm not able to repro that behaviour yet, though.

If I can find a full repro, I'll get back to you.

@dougalg
Copy link
Author

dougalg commented Nov 7, 2019

OK, I've determined what is happening here. It looks like a "not technically a bug" on our end, but definitely "bad behaviour" coupled with this "maybe-a-bug" on your end.

It turns out that we are passing components as props, in some cases. This is very useful for abstract components, and so on. However, it gets weird because we have a prop which is a (poorly defined) object which includes a component as well as other data.

At some point deep in the chain we add a deep watcher on the object that has a component on it. If that component has a VueType, the infinite recursion happens.

Obviously we shouldn't be deep watching that arbitrary object, but just watching the data we actually care about, so I'll see about making that fix in our code.

You can see it happening here: https://codepen.io/dougalg/pen/vYYjxjz?editors=1010

That said, I still feel like this might be worth fixing in the shim file as well? Of course, not sure about the impact on file size it will have.

@dwightjack
Copy link
Owner

Thank you for the detailed explanation.

Ok, I understand your point. I agree with you that maybe deep-watching an object containing a component definition is not the best choice performance-wise.

I also agree that since using the real library does not raise the issue I need to fix the shim to solve that inconsistency.

I don't think the impact on the code will be too big. I didn't consider a use case like yours in the first place so I kept it at the minimum since Vue does not perform prop validation in production (at least not like in production).

In the meantime, there might be a couple of options to fix the problem:

  1. Since deep watching stops at frozen objects (I think that this is the relevant code), you could use Object.freeze on the props object:
const abstractComponent = {
  template: `<p>aaa</p>`,
  props: Object.freeze({
    text: VueTypes.string.def('hello'),
  }),
};
  1. You could define the component prop as a function returning the component definition:
computed: {
     complexProp() {
       return {
         component: () => abstractComponent,
         text: 'wow'
       };
     },
  },

// and 

template: `
    <component
      :is="complexProp.component()"
      :text="complexProp.text"
    />
  `,

I'll keep you updated on the release of a fix.

@dwightjack dwightjack added the bug label Nov 7, 2019
@dwightjack dwightjack self-assigned this Nov 7, 2019
@dougalg
Copy link
Author

dougalg commented Nov 7, 2019

That's great. Thanks a lot!

We already have a temporary fix by removing VueTypes in the broken component. I'm looking into making the deep watcher simpler and more efficient and removing the watch on the internal component completely. As you said, there's no way that it's good for performance!

@dwightjack
Copy link
Owner

Hi,

I published a beta version with a fix:

When you have time, could you check if it solves the issue?

@dougalg
Copy link
Author

dougalg commented Nov 8, 2019

This fixes the issue we were seeing! Thank you for the responsiveness :)

@dwightjack
Copy link
Owner

Cool!

Published as [email protected]. 😃

Feel free to re-open this issue if you encounter this bug again.

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

No branches or pull requests

2 participants