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

Support using result of Vue.extend in mixins #5652

Closed
strantr opened this issue May 11, 2017 · 17 comments
Closed

Support using result of Vue.extend in mixins #5652

strantr opened this issue May 11, 2017 · 17 comments

Comments

@strantr
Copy link
Contributor

strantr commented May 11, 2017

Version

2.3.3

Reproduction link

http://jsfiddle.net/zna06rq4/1/

Steps to reproduce

Run fiddle, error will be shown in console:

Error in callback for watcher "hello": "TypeError: this.cb.call is not a function"

What is expected?

"child" should be logged to the console via the watch handler in Child.

What is actually happening?

The watch is not being fired


In strats.watch (

: [child]
) the function unconditionally converts child to an array.
If this has already been converted to an array (via a previous merge operation) it will convert it to a nested array [[child]].

The fix for this seems to be updating line 175 to have:
Array.isArray(child) ? child : [child]

@dodas
Copy link

dodas commented May 11, 2017

I always do [].concat(whatMightBeArrayOrMightBeNot) in case I need array and it's not obvious if variable is array or not.

@posva
Copy link
Member

posva commented May 11, 2017

As said in the PR, you should use plain objects instead of Vue.extend for mixins: http://jsfiddle.net/yjw4nafL/

You can achieve something similar with extend too:
http://jsfiddle.net/sg0db7no/

@posva posva closed this as completed May 11, 2017
@strantr
Copy link
Contributor Author

strantr commented May 11, 2017

@posva We are using TypeScript with @Component decorators. The implementation of this decorator uses Vue.extend(options) in order to create the components.
We are then needing to extend/mixin functionality from multiple components, causing this issue.

How can we avoid this problem when we do not have the ability to change the underlying component that we have - are you telling me that I cannot use Vue and TypeScript in this scenario when a simple change to the code would fix this?
Thanks

@posva
Copy link
Member

posva commented May 11, 2017

Ideally, the mixin should not be a component, otherwise, you can extend them with Vue.extend but you loose the ability to extend multiple things at the same time

@posva posva reopened this May 11, 2017
@posva posva changed the title Merge of watches via multiple mixins does not work Support using result of Vue.extend in mixins May 11, 2017
@posva
Copy link
Member

posva commented May 11, 2017

Why do you use the @Component decorator if you are exporting a Mixin?

@posva
Copy link
Member

posva commented May 11, 2017

At the same time this should work: http://jsfiddle.net/q1f9f6ts/
So this may be a bug 🤔

@ktsn
Copy link
Member

ktsn commented May 11, 2017

Actually, I implemented mixins for constructors in the past. The motivation was class style declaration is TypeScript friendly than objects.

@posva
Copy link
Member

posva commented May 11, 2017

is it possible to have a different decorator to support the friendly declarations but not wrapping the object with Vue.extend?

@ktsn
Copy link
Member

ktsn commented May 11, 2017

I think wrapping Vue.extend is needed since it simply uses Ctor.options on merging mixins. AFAIK, most of Vue component decorators are using Vue.extend.

@strantr
Copy link
Contributor Author

strantr commented May 11, 2017

There may be some other factors going on here, I've created an example that uses .extend inside a vue-router and it works in Chrome (57.0.2987.133) and not Firefox (53.0.2).

http://jsfiddle.net/sn7p2goh/4/

Error in firefox:

TypeError: this.cb.call is not a function
Stack trace:
run@https://unpkg.com/vue@2.3.3/dist/vue.js:2864:11
flushSchedulerQueue@https://unpkg.com/vue@2.3.3/dist/vue.js:2619:5
queueNextTick/<@https://unpkg.com/vue@2.3.3/dist/vue.js:661:11
nextTickHandler@https://unpkg.com/vue@2.3.3/dist/vue.js:608:7

This was actually the issue that I was having and I traced it to the code that my PR is for, however I do not see what would cause the difference between browsers.

A different decorator would be a possibility, however may not be required if this issue is resolved

@strantr
Copy link
Contributor Author

strantr commented May 12, 2017

I can't work out why it would work in Chrome but not FF, is anyone else able to take a look/any ideas?

@backbone87
Copy link

Just a side note: Vue.extend({}).options will give an error in typescript because typeof Vue doesnt have an option property

@ktsn
Copy link
Member

ktsn commented May 13, 2017

Yes, because it's not declared in declaration files due to undocumented API.

@backbone87
Copy link

Is it considered public API though? It would definitely be helpful for tools/libs/plugins that need reflection over a component or instance to have access to a normalized options object. In the best case with its own NormalizedOptions type/interface.

@strantr
Copy link
Contributor Author

strantr commented May 23, 2017

It seems that the difference in behaviour in Firefox vs all other browsers is caused by vue-router being present on the page:

Sample 1 includes vue-router (external resources) but does not use it - this will error in Firefox
http://jsfiddle.net/sn7p2goh/6/

Sample 2 has vue-router removed - this works
http://jsfiddle.net/sn7p2goh/7/


Update, it is not caused by vue-router as such but by the presence of any global mixin:
http://jsfiddle.net/sn7p2goh/8/

@strantr
Copy link
Contributor Author

strantr commented May 23, 2017

@posva
I've finally figured out the root cause for this issue.
Every object in Firefox already has a "watch" member (Object.prototype.watch).

This means that when it reaches the watch merge strat parent.watch is already true and so it merges into a single array, which my pull request fixes.

I am unsure if I can actually write a unit test for this as the issue is only present in Gecko (only browser with watch support).

@posva
Copy link
Member

posva commented May 23, 2017

Hey, nice job and thanks a lot for looking into it. Well, you can simulate adding the watch property to the Object prototype (and remove it afterwards). Are you sure you opened a PR, I couldn't find it I'm just dizzy and couldn't see it 😓

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

No branches or pull requests

5 participants