Navigation Menu

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

Y.Base.create doesn't aggregate/copy static properties from superclass #773

Open
unkillbob opened this issue May 21, 2013 · 6 comments
Open

Comments

@unkillbob
Copy link
Contributor

Y.Base.create will search the superclass (2nd argument to create()) for the static _buildCfg property to determine how to treat static properties but it doesn't actually copy/aggregate any of the mentioned properties on the superclass, only the mixed in extensions (3rd argument to create()).

This JsFiddle demonstrates the issue: http://jsfiddle.net/MbqLa/1/

Is this behaviour intended? Have I misinterpreted how this is all supposed to work?

@ghost ghost assigned sdesai May 21, 2013
@ericf
Copy link
Member

ericf commented May 21, 2013

It is the intended behavior that _buildCfg is for class extensions. Although, many have gotten hung up on this issue, myself included, and we've talked about making this an enhancement to apply _buildCfg to the subclass.

@unkillbob
Copy link
Contributor Author

Thanks for the clarification Eric. Yeah it seems really odd to me. How do I go about requesting this change?

Also the documentation is misleading as it refers to the Base/Main class declaring the static property (I suspect referring to the actual class being created) however the second argument to create() is labelled the main class. This led me to thinking the static property could be aggregated from the main class in addition to the extensions.

@ericf
Copy link
Member

ericf commented Jul 11, 2013

@unkillbob IIRC, last time @sdesai and I talked about this we thought this is a worthy enhancement. This change is probably tricky to implement, so this ticket can serve as the enhancement request for this change.

@unkillbob
Copy link
Contributor Author

@ericf thanks, in the meantime we've managed to work around it using a custom aggregator in our _buildCfg. Here's that code if it helps:

// Utility function used in `_buildCfg` for aggregating static properties from the given
// supplier to the given receiver ensuring a default value for the property of an empty
// array/object if the receiver does not already declare the property.
function aggregate(propertyName, receiver, supplier) {
    if (!receiver.hasOwnProperty(propertyName) && supplier.hasOwnProperty(propertyName)) {
        receiver[propertyName] = Lang.isArray(supplier[propertyName]) ? [] : {};
    }
    Y.aggregate(receiver, supplier, true, [propertyName]);
}

// Utility function used in `_buildCfg` for aggregating static properties across the given
// base class, the superclass of that base class and the given extension class. This is
// necessary as the default YUI aggregation strategy does not include the superclass,
// only extensions.
function aggregateIncludingSuperclass(propertyName, baseClass, extension) {
    var superclass = baseClass.superclass && baseClass.superclass.constructor;

    if (!baseClass.hasOwnProperty(propertyName) && superclass &&
            superclass.hasOwnProperty(propertyName)) {
        aggregate(propertyName, baseClass, superclass);
    }

    aggregate(propertyName, baseClass, extension);
}

MyExtension._buildCfg = {
    // ensure declared features are aggregated when mixing multiple classes/extensions that
    // declare features
    custom: {
        A_STATIC_PROP: aggregateIncludingSuperclass
    }
};

@cquinders
Copy link

What's the status on ignoring statics in _buildCfg of superclasses? Is there any chance to get this fixed? Took me a while to figure out this bug and come up with this issue.

Perhaps the documentation could at least link to this issue?

@unkillbob Thanks for the code snippet. How did you proceed with this bug? Do you also still use the workaround?

@unkillbob
Copy link
Contributor Author

@cquinders we're still using the workaround

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

4 participants