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

Mixins are not merged correctly for built-in options in this.$options #3566

Closed
Justineo opened this issue Apr 8, 2021 · 10 comments
Closed
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage.

Comments

@Justineo
Copy link
Member

Justineo commented Apr 8, 2021

Version

3.0.11

Reproduction link

SFC Playground

Steps to reproduce

Define a component with options API:

{
  mixins: [
    { computed: { bar: () => 2 } }
  ],
  computed: {
    foo: () => 1
  }
}

What is expected?

this.$options.computed should contain both foo and bar as keys:

this.$options.computed // { foo: () => 1, bar: () => 2 }

What is actually happening?

this.$options.computed // { foo: () => 1 }

It seems to affect all built-in options: https://github.com/vuejs/vue-next/blob/870f2a7ba35245fd8c008d2ff666ea130a7e4704/packages/runtime-core/src/componentOptions.ts#L973

This also makes Devtools unable to show data/computed/... defined in mixins as it relies on this.$options.

@posva
Copy link
Member

posva commented Apr 8, 2021

It seems like Vue 2 does add them to $options https://jsfiddle.net/posva/p2gz45j8/

@Justineo
Copy link
Member Author

Justineo commented Apr 8, 2021

Yeah I've checked it's how devtools currently manage to categorize properties into data, computed, etc.

@Justineo Justineo added the 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. label Apr 8, 2021
@HcySunYang
Copy link
Member

@posva Is this issue the same as the one described by #2119?

@Justineo
Copy link
Member Author

@HcySunYang Yeah I believe both issues rely on options being merged correctly.

@posva
Copy link
Member

posva commented Apr 13, 2021

@HcySunYang I think it's a bit different.

#2119 was initially about being able to retrieve beforeRouteEnter() and other custom options added by the router when the user adds them through mixins. Because they are custom, we would also need a custom merge strategy.

So, IMO solving this issue doesn't require solving the other one and this one is more about consistency with Vue 2 while the other one is about a missing feature given the new behavior and also about devtools. So we should prioritize this one over the other one

@yyx990803
Copy link
Member

Currently devtools for Vue 3 only handles computed/inject keys on the component's base definition, so even if we expose $options like in Vue 2 it still doesn't work.

It's trivial for devtools to account for mixins though, we just need to do a merge on the devtools side for computed/provide/inject here /cc @Akryum

I don't think app code should rely on core merge behavior for $options, and the trade-off isn't worth it considering the extra amount of code needed. The behavior consistency is covered in the migration build, but in proper Vue 3 users should only use $options for custom options, not core options.

@Akryum
Copy link
Member

Akryum commented May 28, 2021

Will take a look tomorrow

@Akryum
Copy link
Member

Akryum commented May 28, 2021

@Akryum
Copy link
Member

Akryum commented May 28, 2021

It seems it doesn't work well for extends though

@yyx990803
Copy link
Member

Yeah currently resolveMergedOptions intentionally does not account for built-in options like computed because it's somewhat wasteful (one of the perf bottlenecks in v2).

What I'm suggesting is for devtools to also check mixins and extends when resolving computed/provide/inject state for an instance.

Akryum added a commit to vuejs/devtools-v6 that referenced this issue May 29, 2021
etherdog-eth added a commit to etherdog-eth/vue-devtools that referenced this issue May 31, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants