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

Adding to this.$options.computed during beforeCreate only works if computed is already defined on component #2791

Closed
MiniGod opened this issue Dec 10, 2020 · 8 comments
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working

Comments

@MiniGod
Copy link

MiniGod commented Dec 10, 2020

Version

3.0.4

Reproduction link

https://jsfiddle.net/6k3nod2c/

Steps to reproduce

Create a component / app with a mixin that adds to this.$options.computed.

const mixin = {
  beforeCreate() {
    if (!this.$options.computed) {
      this.$options.computed = {};
    }
    
    this.$options.computed.value = () => {
    	return 'works';
    }
  }
}

Vue.createApp({
  mixins: [mixin],
  // computed: {} // Uncommenting this line makes it work
}).mount('#app')

What is expected?

Expect this.value to be "works".

What is actually happening?

this.value is undefined.


In vue 2 this worked.
In vue 3 this only works if the computed option is already defined.

@LongTengDao
Copy link
Contributor

LongTengDao commented Dec 10, 2020

only components change could be expected work right in beforeCreated, not including dynamic computed props inject.

because they are pre defined in the prototype, not define for each instance. and only root instance (new Vue / Vue.createApp) won't do that, component in compontents or registered in Vue.component or extended by Vue.extend, they will all do that for performance.

why not do below simply in your sample?:

const mixin = {
  computed: {
    value: () => 'works'
  }
}

@posva posva added 🐞 bug Something isn't working 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Dec 11, 2020
@MiniGod
Copy link
Author

MiniGod commented Dec 15, 2020

The example was a simplification, so it's not that simple. I encountered this bug when upgrading from Vue 2 where this used to work.

In our app we have a global mixin that looks at another $options to generate computed properties.

Something like:

const mixin = {
  beforeCreate() {
    const magicOptions = this.$options.magic;

    for (const alias in magicOptions) {
      // subscribe to kv store
      const getterAndSetter = subscribe(magicOptions[alias]);

      this.$options.computed[alias] = getterAndSetter;
    }
  },
  unmounted() {
    // unsubscribe when 
    this.$options.magic.forEach(unsubscribe);
  }
}

Then any component can define a "magic" object to subscribe to the backend, and subscription is stopped when component is removed.

export default {
  magic: {
    state: 'some-key-in-the-kv-store',
  },
  mounted() {
    console.log('state', this.state);
  },
  methods: {
    start() {
      this.state = 'start';
    }
  }
}

I might be doing this the wrong way, so any suggestions are welcome.

@luwuer
Copy link
Contributor

luwuer commented Dec 17, 2020

@edison1105 I think your solution is not completely,take a look about this #2839

@LongTengDao
Copy link
Contributor

LongTengDao commented Dec 18, 2020

The example was a simplification, so it's not that simple.

I see. I think add a hook beforeExtend which run once for each component would really help. There has been many thing indirect and inefficient for plugins via beforeCreate.

@luwuer
Copy link
Contributor

luwuer commented Dec 18, 2020

I see. I think add a hook beforeExtend which run once for each component would really help. There has been many thing indirect and inefficient for plugins via beforeCreate.

Add a lifecycle hook beforeExtend seems like not a recommendable resolve:

  1. User have to modify code because its inconsistent with vue2
  2. Ambiguous because its overlap with beforeCreate
  3. Efficiency has not change because extends and mixins can also hold this hook(In fact i don't understand why beforeCreate is indirect and inefficient)

@aethr
Copy link

aethr commented Jan 18, 2021

We ran into the same issue in our app where we're creating a validation helper similar to Vuelidate.

We define a validations option in a component when we want some data properties to be validated, and a global mixin will add this to a computed property (so that validation rules can rely on other data properties with reactivity).

We ran into two problems:

  1. As mentioned in this issue, if we didn't add an empty computed definition to our mixin, adding a dynamic computed property in beforeCreate didn't work. This seems due to how options is destructured at the start of applyOptions: https://github.com/vuejs/vue-next/blob/master/packages/runtime-core/src/componentOptions.ts#L478-L509

I believe this could be fixed by only defining computedOptions immediately before it's used: https://github.com/vuejs/vue-next/blob/master/packages/runtime-core/src/componentOptions.ts#L647

const computedOptions = options.computed;
if (computedOptions) {
...

This way if a beforeCreate hook added a computed object to options dynamically, it should be accessible here.

  1. When we tried adding a computed definition to the global mixin, the computed object reference was shared between all component instances

For example, adding to the example from @MiniGod:

const mixin = {
  computed: {},
  beforeCreate() {
    if (condition) {
      this.$options.computed.example = () => {
      	return 'works';
      }
    }
  }
}

In theory this fixes the issue, however since mixin is defined as a const in our module, the reference to the mixin.computed object is shared among all component instances that apply the mixin. Then when the condition passes for any instance, and example is added to this.$options.computed, it is added to the referenced object, and hence it gets added to every instance created afterwards.

This may be something to be added in a separate issue, I only bring it up because it's a major issue in the only workaround I can see to this issue.

@valq7711
Copy link

I found a terrible (but working) solution:
define a property directly on an instance

// in beforeCreate-hook
const foo = computed(getter/setter)
Object.defineProperty(this, 'foo', {
    enumerable: true,
    configurable: true,
    get: ()=> foo.value,
    set: (v)=> foo.value=v
})

@yyx990803
Copy link
Member

This has been fixed by e2ca67b

@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working
Projects
None yet
7 participants