yui / yui3

A library for building richly interactive web applications.
http://yuilibrary.com/
Other
4.12k stars 1.29k forks source link

[Loader] Sorting modules incorrectly when YUI.GlobalConfig.groups is configurated #639

Open springuper opened 11 years ago

springuper commented 11 years ago

When we upgrade our site from 3.5.1 to 3.8.0, a strange error occurs again and again. The code is very simple:

var nlAddCart = Y.all('.add-cart');
nlAddCart.on('click', function (e) {
    e.halt();
    // do sth
});

There are two items in nlAddCart, but neither acts as we wish when a click event happens.

I spend one day to find out the reason, and finally get it:

Step 1: YUI.GlobalConfig is initialized with a object o, which has a groups property to contain all our modules:

YUI.GlobalConfig = {
    // ...
    groups: {
        fecore: {
            'mt-base': {
                requires: ['yui', 'node-base'],
                path: //...
            }
        }
    }
}

Step 2: when a new YUI instance Y is created, o.groups is mixed to Y.config

Step 3: when Y.use is called for the first time, a new Y.Loader instance, named loader, is created with Y.config. loader._config is automatically called in the construction process, and then loader.addGroup, and then loader.addModule,

// https://github.com/yui/yui3/blob/v3.8.0/build/yui/yui.js#L6653
this.moduleInfo[name] = o;

all module meta on YUI.GlobalConfig.groups is assigned to loader.moduleInfo.

Step 4: in the loader._explode method (explode all modules directly or indirectly required by used modules, it is called by loader.calculate in Y.use), an important method loader.getRequires is called to collect all modules directly or indirectly required by current module, and assign this collection result to the module's property expanded/expanded_map. Therefore, the meta in YUI.GlobalConfig is changed globally. To optimize performance, loader.getRequires detects some features to avoid needless computing:

// https://github.com/yui/yui3/blob/v3.8.0/build/yui/yui.js#L7012
reparse = !((!this.lang || mod.langCache === this.lang) && (mod.skinCache === this.skin.defaultSkin));

if (mod.expanded && !reparse) {
    return mod.expanded;
}

Step 5: in the loader._sort method called after loader._explode, a comparison method loader._require(mod1, mod2) is called for each pair of modules to determine which should be put ahead. The expanded/expanded_map property of a module plays a big role.

Step 6: in the first YUI.use('mt-base') statement, all modules required by 'mt-base' are processed by loader.getRequire method and consequently gets the expanded/expanded_map property. But when YUI.use('mt-base') runs in the second time, as 'mt-base' already has expanded, skinCache, and langCache properties, loader.getRequire returns expanded result directly according to Step 4. Therefore, every YUI built-in modules required by 'mt-base' havs no expanded property, finally, loader._sort method computes a wrong sequence. In this example, the order is:

["yui", "node-base", "event-base", "event-custom-base", "oop", "yui-base", "node-core", "dom-core", "features", "selector", "selector-native", "dom-base", "mt-base"]

This causes Y._attach process 'event-base' before 'dom-core' and 'dom-base', and further results in shouldIterate method gets a wrong answer. So, nlAddCart.on fails.

In summary, this problem is caused by two points:

The full code:

YUI.GlobalConfig = {
    root: '...',
    combine: true,
    comboBase: '...',
    groups: {
        fecore: {
            'mt-base': {
                requires: ['yui', 'node-base'],
                path: '...'
            }
        }
    }
};

YUI().use('mt-base', function (Y) {
    // do sth
});
YUI().use('mt-base', function (Y) {
    var nlAddCart = Y.all('.add-cart');
    nlAddCart.on('click', function (e) {
        e.halt();
        // do sth
    });
});
caridy commented 11 years ago

This might be related to issue #559

springuper commented 11 years ago

No one should deal with this bug? We have hacked through our YUI().use wrapper, but it's really strong desired for YUI team to repair this problem.

ezequiel commented 10 years ago

@springuper,

does this problem still happen on yui 3.17.2? we recently rewrote loader's _sort algorithm, so your problem may have been inadvertently fixed. it no longer uses expanded, expanded_map, or _requires