Vuec causing component props to be bound incorrectly. #5

defnorep · 2017-05-25T00:41:20Z

Hello again!

I believe I have found another edge case. I am able to reproduce the problem fairly easily, see further below for code.

I have a component. That component has props. I create two instances of this component in the template, each with different values for the same props. The component renders the expected prop values, and the Vue DevTools also display the correct prop values.

However, any reference to these props inside of the lifecycle hooks only returns the values of the props passed to the first component instantiated.

I have narrowed this down to the Vue Container. If I change line 6 of src/Vue.js to the following:
hooks[hooks.length - 1] = hooks[hooks.length - 1], this unexpected behavior goes away. So it seems that container.prepare is somehow binding to the wrong Vue instance, though this is the part that I can't narrow down. It all seems to be correct. Maybe there is some other type of interaction that I'm not able to see right away.

I also noticed that component methods were getting the wrong scope resolution (though computed properties do not). Maybe this indicates an unexpected interaction with lifecycle initialisation, since there is quite a bit of magic done by Vue within that process.

Inspecting this during the Vue.mixin call results in all distinct components, so it seems we can rule that out...

import Vue from 'vue';
import Vuec from 'vue-container';
import PropTest from '@/{path}/prop-test';

// Set up the IOC container.
Vue.use(Vuec);

/* eslint-disable no-new */
new Vue({
  el: '#vue-app',
  components: {
    PropTest
  },
});

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>test</title>
</head>
<body>
    <div id="vue-app">
        <prop-test a="111" :b="111" c></prop-test>
        <prop-test a="222" :b="222"></prop-test>
    </div>
</body>
</html>

<template>
  <div class="prop-test">
    {{ a }}
    {{ b }}
    {{ c }}
    {{ computedScope }}
    {{ methodScope() }}
  </div>
</template>

<script>
/*eslint-disable*/
export default {
  data() {
    return {};
  },
  created() {
    console.log(this._uid, 'created hook', this.a, this.b, this.c);
  },
  mounted() {
    console.log(this._uid, 'mounted hook', this.a, this.b, this.c);
  },
  computed: {
    computedScope() {
      console.log(this._uid, 'computed scope', this.a, this.b, this.c);
      return 'noop';
    },
  },
  methods: {
    methodScope() {
      console.log(this._uid, 'method scope', this.a, this.b, this.c);
      return 'noop';
    },
  },
  props: {
    a: {
      type: String,
    },
    b: {
      type: Number,
    },
    c: {
      type: Boolean,
      default: false,
    },
  },
};
</script>

<style lang="scss" scoped>
</style>

The text was updated successfully, but these errors were encountered:

dealloc · 2017-05-25T09:24:14Z

Hey, thank you very much for reporting and investigating this problem!

I'm glad to see you're still using Vuec!
I'll get into this issue as soon as possible

defnorep · 2017-05-25T14:26:07Z

I'm looking at this in more depth this morning.

I tried the service injection as opposed to the parameterized hook injection. Same behaviour, though removing the patchHook calls does fix it. If we have a hard time figuring out how to fix this problem, we could allow for injection via service object only by conditionally turning off the parameterized injection if it is used. This is only a workaround though.

Although, if parameterized injection doesn't allow for name mangling, and it has a strange interaction with Vue's lifecycle initialisation, it might just be prudent to completely remove it and make the services object the only way.

Addition 1:

If you add another hook to the mixin call and inspect this, you can see the problem.

Vue.mixin({
  beforeCreate() {
    injectServices(this, container);
    patchHook(this, 'created', container);
    patchHook(this, 'beforeMount', container);
    patchHook(this, 'mounted', container);
    patchHook(this, 'beforeUpdate', container);
    patchHook(this, 'updated', container);
    patchHook(this, 'beforeDestroy', container);
    patchHook(this, 'destroyed', container);
  },
  // add this
  created() {
    console.log(this);
  },
});

Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}

Go a step further and remove the patchHook call to the created hook, and you'll see the change in behavior (see the component's UID):

Vue$3 {_uid: 0, _isVue: true, $options: Object, _renderProxy: Proxy, _self: Vue$3…}
Vue.js?69df:37 VueComponent {_uid: 1, _isVue: true, $options: Object, _renderProxy: Proxy, _self: VueComponent…}
Vue.js?69df:37 VueComponent {_uid: 2, _isVue: true, $options: Object, _renderProxy: Proxy, _self: VueComponent…}
Vue.js?69df:37 VueComponent {_uid: 3, _isVue: true, $options: Object, _renderProxy: Proxy, _self: VueComponent…}
Vue.js?69df:37 VueComponent {_uid: 4, _isVue: true, $options: Object, _renderProxy: Proxy, _self: VueComponent…}
Vue.js?69df:37 VueComponent {_uid: 5, _isVue: true, $options: Object, _renderProxy: Proxy, _self: VueComponent…}

Addition 2:

This is shipped as a plugin, which encapsulates a global mixin (Vue.mixin()). Vue's documentation states that calling the same plugin more than once will have no effect. Vue probably has some magic where it will only ever apply a mixin to a component type once. I don't think that resolving scope of this in a global mixin is going to work for this use case, since it will only ever resolve this once.

dealloc · 2017-05-25T16:13:05Z

There's some really odd stuff going on, because a simple test that I'd expect to fail:

it('Should bind on the correct instances.', function () {
	var uuid = 0;
	new Vue({
		el: '#app',
		template: '<div id="app2"></div>',
		mounted: function () {
			uuid = this._uid;
		}
	});
	new Vue({
		el: '#app2',
		mounted: function () {
			assert.notEqual(this._uid, uuid);
		}
	});
});

which actually passes, meaning that inside their hooks they have the correct ID, but indeed as you mentioned adding a "created" hook inside the mixing will cause it to only ever see uid 0.

I'll have to look into this for now, could you create a separate project demonstrating the issue? I'm currently nearing my finals and I'm afraid that I can't spare as much time to set one up as I'd like

defnorep · 2017-05-25T17:16:19Z

Oops, that close button is easy to click. Sorry.

Here's a vue-cli project that displays the problem:
https://github.com/daekano/vuec-props-test

Check out the console. Methods and computed properties are bound correctly, as are calls to beforeCreate(). All other hooks are wrong.

dealloc · 2017-05-26T09:51:56Z

I don't think it's related to properties itself (though they are a side effect of the problem), but rather that for some reason the bound methods only get bound to the first instance that gets bound (so that all hooks get called in the context of the first instance and thus it appears the second instance has the properties of the first).

I'll have to dig into the source code of VueJS again to see how they handle the binding of their methods.
It's quite odd I must say, I'm going to see if I can write a unit test that fails on this problem.

defnorep · 2017-05-26T12:59:46Z

Haha, yeah I was digging around in there too. Specifically these three files:

https://github.com/vuejs/vue/blob/dev/src/core/instance/init.js

https://github.com/vuejs/vue/blob/dev/src/core/global-api/use.js

https://github.com/vuejs/vue/blob/dev/src/core/global-api/mixin.js

It's not immediately apparent though. And we know Vue does some magic here and there, proxy objects and performance references, etc.

In the interim, I've taken your library as inspiration and have written a simplified solution just to unblock my team. It goes the $services and inject() path since scope binding inside plugins seems to be problematic. I still want to see Vuec become the de facto container for the VueJS ecosystem, but this way you should feel no pressure from me at least.

This commit is an attempt to identify and fix an issue regarding the bound instance

dealloc · 2017-06-01T13:36:44Z

Hey, sorry for the delay; I've been crazy busy with finishing my internship and signing a contract with another company in between my exams and all, my apologies for the radio silence.

Could you check if 79d957e solves anything for you?

defnorep · 2017-06-01T19:43:15Z

Congratulations!

I will not be able to test this until I return from my vacation in three weeks. I write to you from the airport 😂

Til then!

dealloc · 2017-06-01T19:45:09Z

I will try to confirm and fix the bug further myself then, thank you for the assistance!

Have a great vacation

dealloc · 2017-09-01T19:23:15Z

@Daekano can you check the patch I wrote for you?

kashiif · 2017-10-11T20:42:41Z

Hi @dealloc, thanks for this awesome library.

I ran into the same problem as @Daekano. I tested 79d957e on vuec-props-test repo. It does not fix the problem. I always see the 111 true for all hooks even for the second component instance where it should have been 222 false.

The problem is, as you stated in the this thread

for some reason the bound methods only get bound to the first instance

The cause of the problem is this line in the file src/Vue.js:

    const hook = hooks[hooks.length - 1].bind($vm);

I found that the hooks[hooks.length - 1] function is already a bound function using this technique. Calling bind with one argument on an already bound function has no effect (Here is a little snippet to prove this). For this reason, the above statement has no effect.

Now, for the test you mentioned:

it('Should bind on the correct instances.', function () {
	var uuid = 0;
	new Vue({
		el: '#app',
		template: '<div id="app2"></div>',
		mounted: function () {
			uuid = this._uid;
		}
	});
	new Vue({
		el: '#app2',
		mounted: function () {
			assert.notEqual(this._uid, uuid);
		}
	});
});

This does not cover the problematic scenario. The problem arises only when multiple instances of the same component definition are mounted (either simultaneously or in succession).

I have sent you PR to fix this issue. This PR

Fixes the issue. I have verified this on my code and vuec-props-test repo.
Updates the test that now fails on master branch but passes on the PR.

I would be glad if @Daekano could verify this as well.

dealloc · 2017-10-11T21:42:24Z

@kashiif great catch! Seems one learns every day.
I have approved the changes in your PR, if we can get @Daekano to verify I'll merge it in!

Regardless, thank you for taking the time to make a PR

Fix #5: lifecycle hooks must be called on correct instance

defnorep · 2017-10-30T19:34:10Z

@dealloc My apologies! I've since started using a different email address and haven't been active on GitHub. I will endeavour to test this today.

Thanks for the reminder @kashiif :D

Edit: Looks like you merged this already. Do you still need me to test?

dealloc · 2017-10-30T20:48:48Z

@Daekano if you could test if this solves (some of) your problems, that'd be great :)

defnorep · 2017-10-30T22:28:13Z

@dealloc Okay I'll try to reproduce the issue and apply the fix.

I decided to build our own IOC container to remove the dependency since it's not all that complicated anyways, so my problem environment has since disappeared.

dealloc · 2017-10-30T22:29:24Z

That'd be perfect. And you can always use the Vuec container (it's in a separate file and has no dependencies either ;) )

defnorep · 2017-11-08T16:16:37Z

@dealloc This is fixed!

Again, apologies for the delay. I'm quite busy these days and haven't formed any open source habits, so it tends to escape my mind.

dealloc · 2017-11-08T16:22:17Z

No problem, thanks for checking it!

Thanks a lot @kashiif for the patch!

kashiif · 2017-11-10T00:29:06Z

@dealloc Any plan for releasing this fix?

dealloc · 2017-11-10T09:52:44Z

@kashiif of course, my bad! I've been sick this week and haven't really given it much thought.

Expect a new release within a couple hours!

dealloc · 2017-11-10T11:53:24Z

@kashiif I have released v1.1.1 which includes your fix both on Github and NPM

kashiif · 2017-11-22T19:10:54Z

Belated thanks for this release. Works like a charm.

MisterGoodcat · 2018-02-09T15:17:54Z

Hey guys. I found this project and this issue because I'm building a TypeScript-based DI container with an adapter to Vue.js and ran into a similar problem.

I justed wanted to let you know that the proposed solution here does not fully/cleanly resolve this issue. What's happening here is no Vue.js magic, it's just plain JavaScript mechanics. Let me explain: the hooks you pull from $options are located in the prototype of the options, not in the options instance of the component itself. The first time you patch them works OK, but the second time you're patching the already patched hook again. So the second time a component of a type is created, its hook points to the previously patched hook which points to the original one. And so on. Since in your previous code all patched hooks had been bound to the respective component at creation time, this simply meant that the most inner call (the first hook created) won with regards to the used context. That's why you were always only seeing the first component instance.

Vue.js calls patched "created" (bound to component 2) 
  => which calls patched "created" (bound to component 1)
    => which calls the original "created" (which is now bound to component 1, not component 2)

That's also the reason your current solution is questionable: technically it works because you remove the explicit binding and pass on the outer-most component down the chain. But you still increase the involved call stack by 1 every time a component is created, which surely is no good for performance and will even crash at some point (max call stack depth).

Replacing the hook can be a one-time thing (per component type).

MisterGoodcat · 2018-02-09T15:21:17Z

Btw. I haven't looked at your code in much detail, but it seems to me that the way it's implemented now it will also have lifetime issues for the resolved dependencies, i.e. if you ever intend to support transient instances I'm pretty sure they won't resolve correctly with the current approach of patching the hooks.

dealloc self-assigned this May 25, 2017

dealloc added the bug label May 25, 2017

defnorep closed this as completed May 25, 2017

defnorep reopened this May 25, 2017

dealloc added a commit that referenced this issue Jun 1, 2017

working on #5

79d957e

This commit is an attempt to identify and fix an issue regarding the bound instance

kashiif pushed a commit to kashiif/vuec that referenced this issue Oct 11, 2017

Fix dealloc#5: lifecycle hooks must be called on correct instance

5460dd1

dealloc added needs confirmation PR pending labels Oct 11, 2017

dealloc added a commit that referenced this issue Oct 15, 2017

Merge pull request #6 from kashiif/dealloc/issue-5

8cd303f

Fix #5: lifecycle hooks must be called on correct instance

dealloc closed this as completed Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vuec causing component props to be bound incorrectly. #5

Vuec causing component props to be bound incorrectly. #5

defnorep commented May 25, 2017 •

edited

Loading

dealloc commented May 25, 2017

defnorep commented May 25, 2017 •

edited

Loading

dealloc commented May 25, 2017

defnorep commented May 25, 2017

dealloc commented May 26, 2017

defnorep commented May 26, 2017

dealloc commented Jun 1, 2017

defnorep commented Jun 1, 2017

dealloc commented Jun 1, 2017

dealloc commented Sep 1, 2017

kashiif commented Oct 11, 2017

dealloc commented Oct 11, 2017

defnorep commented Oct 30, 2017 •

edited

Loading

dealloc commented Oct 30, 2017

defnorep commented Oct 30, 2017

dealloc commented Oct 30, 2017

defnorep commented Nov 8, 2017 •

edited

Loading

dealloc commented Nov 8, 2017

kashiif commented Nov 10, 2017

dealloc commented Nov 10, 2017

dealloc commented Nov 10, 2017

kashiif commented Nov 22, 2017

MisterGoodcat commented Feb 9, 2018

MisterGoodcat commented Feb 9, 2018

Vuec causing component props to be bound incorrectly. #5

Vuec causing component props to be bound incorrectly. #5

Comments

defnorep commented May 25, 2017 • edited Loading

dealloc commented May 25, 2017

defnorep commented May 25, 2017 • edited Loading

dealloc commented May 25, 2017

defnorep commented May 25, 2017

dealloc commented May 26, 2017

defnorep commented May 26, 2017

dealloc commented Jun 1, 2017

defnorep commented Jun 1, 2017

dealloc commented Jun 1, 2017

dealloc commented Sep 1, 2017

kashiif commented Oct 11, 2017

dealloc commented Oct 11, 2017

defnorep commented Oct 30, 2017 • edited Loading

dealloc commented Oct 30, 2017

defnorep commented Oct 30, 2017

dealloc commented Oct 30, 2017

defnorep commented Nov 8, 2017 • edited Loading

dealloc commented Nov 8, 2017

kashiif commented Nov 10, 2017

dealloc commented Nov 10, 2017

dealloc commented Nov 10, 2017

kashiif commented Nov 22, 2017

MisterGoodcat commented Feb 9, 2018

MisterGoodcat commented Feb 9, 2018

defnorep commented May 25, 2017 •

edited

Loading

defnorep commented May 25, 2017 •

edited

Loading

defnorep commented Oct 30, 2017 •

edited

Loading

defnorep commented Nov 8, 2017 •

edited

Loading