Skip to content

Component multiple extension data inheritance broken in 0.11 #594

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Jabher opened this issue Nov 25, 2014 · 16 comments
Closed

Component multiple extension data inheritance broken in 0.11 #594

Jabher opened this issue Nov 25, 2014 · 16 comments

Comments

@Jabher
Copy link

Jabher commented Nov 25, 2014

following technic not working in 0.11

var WidgetComponent = Vue.component({
data: function(){return {items: []}}
});
var WidgetWithCounterComponent = WidgetComponent.extend({
data: function(){return {count: 0}}
});

Data is now a function that gets overriden, so that this.items becomes not available.
Not quite sure if it's correct behaviour, as I've used this technic before and it was OK.

Using special merge function as a solution, but have a feeling that i'm doing it in wrong way.

Still, did not try to use mixins for object extension yet - migrated to 0.11 yesterday. Great work, btw, extreme performance increase, up to 2 or 3 times in most of the app bottlenecks.

@yyx990803
Copy link
Member

Thanks, this should indeed be the correct behavior. Also glad to know it's improving performance for your app :)

@Jabher
Copy link
Author

Jabher commented Nov 25, 2014

Ok, got it.
So, what is the right way to solve this kind of case?
Creating a component scratch (base component to extend) with data is extremely useful in some of the cases.

@yyx990803
Copy link
Member

Oh, I mean your expected behavior should be the correct one, so I fixed it in yyx990803/vue@829a610

@agonbina
Copy link

Hmmm, it seems like when there is more than one mixin listed in a Component definition, adding new keys to the same object in data: function() {} does not work. Take a look at this piece of code:

var firstMixin = {
    data: function() {
        return { 
            $sortable: { cursor: 'move' } 
        }    
    }
}

var secondMixin = {
    data: function() {
        return { 
            $sortable: { handle: '.handle' } 
        }   
    }
}

var MyComponent = Vue.extend({
    mixins: [ firstMixin, secondMixin ],

    data: function() {
        return {
            $sortable: {
                notMerged: 'value' // This value will not be merged into $sortable
            }
        }
    },

    ready: function() {
        console.log('Data:', this.$data.$sortable)           
    }
})

new MyComponent().$mount('#app')

I tried this on the #dev branch.

@yyx990803
Copy link
Member

Yeah, the merging only merges top-level properties, it doesn't merge recursively. I've made it recursive in the commit below.

@agonbina
Copy link

A W E S O M E 👍

p.s. This is unrelated to this issue but it might be another bug maybe. I tried logging '$sortable' using .$log('$sortable') but it throws with this message message: "Unexpected token u"

Is it a bug or is it not possible to log keypaths that are not observed?

@yyx990803
Copy link
Member

@agonbina indeed, fixed here: 8899672

@agonbina
Copy link

agonbina commented Dec 1, 2014

@yyx990803 👍

Also, whenever I try to set a property that starts with an _ or a $ sign I get this warning message: Refused to $add reserved key: ...propertyName... and the properties are not set on the view model instance.

This happens only on Component instances that are children of another component, ex: Column is a child of Grid, you can set a property with _ or $ on the Grid instances, but not Column if they are nested under Grid. I'm using Vue 0.11.1

@yyx990803
Copy link
Member

Can you make a fiddle for the reserved key problem?

@agonbina
Copy link

agonbina commented Dec 1, 2014

Hmm, I'm having difficulties reproducing the problem in a jsFiddle. Does the 0.11.1 release on npm has the exact same code as the cdnjs dist ?

[edit] Well I replaced the npm Vue package with the cdnjs source, exposing Vue as a global instead and this problem is gone.

I'd still like to know if the npm package has the latest code though, or the cdnjs release is newer?

@yyx990803
Copy link
Member

They should be the same. This is weird though. Or maybe you can create a clone-able repo using the npm version that demonstrates the issue?

@agonbina
Copy link

agonbina commented Dec 1, 2014

Ok, here is the repo with the problem, just clone it and do an npm install to get Vue and Webpack: https://github.com/agonbina/reserved-issue

You can also uncomment the script tag which includes the cdnjs version, then comment out the requiring of vue in the main.js file and build it, to see this problem go away.

Although the cdnjs version has its own issue: it doesn't merge the 'reserved' properties if a component is nested within another component, in this case Column inside of Grid.

@yyx990803
Copy link
Member

Hah, actually it's because you were using the minified production version from the cdn, which suppresses all the warnings.

But the reserved key warning is a leftover from an old design, which is no longer needed. I have removed it in dev.

@agonbina
Copy link

agonbina commented Dec 2, 2014

@yyx990803 oh wow ... well then hopefully this is my last question on this issue: is viewmodel.$get not supposed to work with non observed keys? I am able to retrieve them using $data.$reservedKey.property but not with .$get('$reservedKey.property')

@yyx990803
Copy link
Member

I'm afraid that's a limitation you have to work with... reserved keys are not proxied on the vm (because of internal methods/properties), so they cannot be retrieved via $get (which has to be called on the vm for possible prototypal inheritance to parent scope)

@agonbina
Copy link

agonbina commented Dec 2, 2014

Understandable 👍

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

No branches or pull requests

3 participants