Skip to content

Inheritted group combo params #1838

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.

Sign up for GitHub

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

Open
andrewnicols wants to merge 2 commits into
base: dev-3.x
Choose a base branch
from
Open

Inheritted group combo params #1838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b970343
The group comboBase should inherit from the default if no group setti…
andrewnicols May 22, 2014
1473f2e
Groups should inherit combo-related settings from default
andrewnicols May 22, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/loader/HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
YUI Loader Change History
=========================

@VERSION@
3.18.0
Copy link
Member

@juandopazo juandopazo May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leave it as @VERSION@. Grunt handles this replacement.

Copy link
Contributor Author

@andrewnicols andrewnicols May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah /o\

------

* No changes.
* Inherit group settings from base if they were not specified. ([#1838][]: @andrewnicols)

3.17.2
------

* Fix a bug in 3.17.1 where there comboBase was no longer inheritted from the default group. ([#1837][]: @andrewnicols)

3.17.1
------
Expand Down
25 changes: 21 additions & 4 deletions src/loader/js/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2637,6 +2637,7 @@ Y.log('Undefined module: ' + mname + ', matched a pattern: ' +
var inserted = (self.ignoreRegistered) ? {} : self.inserted,
comboSources = {},
maxURLLength,
combine,
comboMeta,
comboBase,
comboSep,
Expand All @@ -2654,10 +2655,17 @@ Y.log('Undefined module: ' + mname + ', matched a pattern: ' +

group = self.groups[mod.group];

// Set sensible defaults for these on each iteration to ensure we don't leak.
combine = self.combine;
comboBase = self.comboBase;
comboSep = self.comboSep;
maxURLLength = self.maxURLLength;

if (group) {
if (!group.combine || mod.fullpath) {
if (typeof group.combine === 'boolean') {
combine = group.combine;
}
if (!combine || mod.fullpath) {
//This is not a combo module, skip it and load it singly later.
addSingle(mod);
continue;
Expand All @@ -2668,9 +2676,18 @@ Y.log('Undefined module: ' + mname + ', matched a pattern: ' +
mod.root = group.root;
}

comboBase = group.comboBase;
comboSep = group.comboSep;
maxURLLength = group.maxURLLength;
if (typeof group.comboBase === 'string') {
Copy link
Member

@juandopazo juandopazo May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about comboBase: ''?

Copy link
Contributor Author

@andrewnicols andrewnicols May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't we? It should be possible to either specify a comboBase for a group, or to use the inheritted one. There are reasons you may want to do so, largely developer related admittedly.

Copy link
Member

@juandopazo juandopazo May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a personal stance in this case. But should we stick to backwards compatibility ('' being falsy)?

Copy link
Member

@caridy caridy May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guys, let's stick to the previous behavior, there is not need to change that behavior now.

comboBase = group.comboBase;
}
if (comboBase !== self.comboBase) {
// It only makes sense to use a different combo loader settings if the comboBase is different.
if (typeof group.comboSep === 'string') {
comboSep = group.comboSep;
}
if (typeof group.maxURLLength !== 'undefined') {
Copy link
Member

@juandopazo juandopazo May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number?

Copy link
Contributor Author

@andrewnicols andrewnicols May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if maxURLLength is a string containing a Number?

Copy link
Member

@juandopazo juandopazo May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YUI is pretty biased towards strict typing. Let's go with the trend.

Copy link
Member

@caridy caridy May 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, previous behavior.

maxURLLength = group.maxURLLength;
}
}
} else {
if (!self.combine) {
//This is not a combo module, skip it and load it singly later.
Expand Down
158 changes: 158 additions & 0 deletions src/loader/tests/unit/assets/loader-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,164 @@ YUI.add('loader-tests', function(Y) {
Assert.isTrue(out.js.indexOf('http://secondhost.com/combo?3.5.0/foogg/foogg-min.js') >= 0, 'Group combo URL should be included in the result');
Assert.isTrue(out.js.indexOf('http://yui.yahooapis.com/combo?3.5.0/cookie/cookie-min.js') >= 0, 'Default YUI combo URL should be included in the result');
},
'test inheritted comboBase with groups': function () {
var loader = new testY.Loader({
combine: true,
groups: {
testGroup: {
modules: {
foogg: {
requires: []
}
}
}
},
require: ['foogg', 'cookie']
});
var out = loader.resolve(true);
Assert.areSame(1, out.js.length, 'Loader generated multiple URLs for a single comboBase');

var url = out.js[0];
Assert.isArray(url.match(/3.5.0\/foogg\/foogg-min\.js/), 'Group match should be combo-loaded with the default URL');
Assert.isArray(url.match(/3.5.0\/cookie\/cookie-min\.js/), 'Default match should combo-load with the group result');
},
'test combine with groups': function () {
var loader = new testY.Loader({
combine: false,
groups: {
testGroup: {
combine: true,
comboBase: 'http://secondhost.com/combo?',
modules: {
foogg: {
requires: []
}
}
}
},
require: ['foogg', 'cookie']
});
var out = loader.resolve(true);
Assert.areSame(2, out.js.length, 'Loader did not generate one URL per comboBase');
Assert.isTrue(out.js.indexOf('http://secondhost.com/combo?3.5.0/foogg/foogg-min.js') >= 0, 'Group combo URL should be included in the result');
Assert.isTrue(out.js.indexOf('http://yui.yahooapis.com/3.5.0/cookie/cookie-min.js') >= 0, 'Default YUI combo URL should be included in the result');
},
'test inheritted combine with groups': function () {
var loader = new testY.Loader({
combine: true,
groups: {
testGroup: {
modules: {
foogg: {
requires: []
}
}
}
},
require: ['foogg', 'cookie']
});
var out = loader.resolve(true);
Assert.areSame(1, out.js.length, 'Loader generated multiple URLs for a single comboBase');

var url = out.js[0];
Assert.isArray(url.match(/3.5.0\/foogg\/foogg-min\.js/), 'Group match should be combo-loaded with the default URL');
Assert.isArray(url.match(/3.5.0\/cookie\/cookie-min\.js/), 'Default match should combo-load with the group result');
},
'test inheritted comboSep with groups': function () {
var loader = new testY.Loader({
combine: true,
comboSep: '__PLACEHOLDER__',
groups: {
testGroup: {
modules: {
foogg: {
requires: []
},
boogg: {
requires: []
}
}
}
},
require: ['foogg', 'boogg', 'cookie', 'dom-screen']
});
var out = loader.resolve(true);
Assert.areSame(1, out.js.length, 'Loader generated a single URL for a single comboBase');

var url = out.js[0];
Assert.isArray(url.match(/__PLACEHOLDER__/), 'Group should inherit the comboSep');
},
'test inheritted comboSep with groups if comboBase is the same': function () {
var loader = new testY.Loader({
combine: true,
groups: {
testGroup: {
comboSep: '__PLACEHOLDER__',
modules: {
foogg: {
requires: []
},
boogg: {
requires: []
}
}
}
},
require: ['foogg', 'boogg', 'cookie', 'dom-screen']
});
var out = loader.resolve(true);
Assert.areSame(1, out.js.length, 'Loader generated a single URL for a single comboBase');

var url = out.js[0];
Assert.isNull(url.match(/__PLACEHOLDER__/), 'Group should inherit the default comboSep');
},
'test different comboSep within group if comboBase is different': function () {
var loader = new testY.Loader({
combine: true,
groups: {
testGroup: {
comboBase: 'http://secondhost.com/combo?',
comboSep: '__PLACEHOLDER__',
modules: {
foogg: {
requires: []
},
boogg: {
requires: []
}
}
}
},
require: ['foogg', 'boogg', 'cookie', 'dom-screen']
});
var out = loader.resolve(true);
Assert.areSame(2, out.js.length, 'Loader generated multiple URLs for a single comboBase');

Assert.isTrue(out.js.indexOf('http://secondhost.com/combo?3.5.0/foogg/foogg-min.js__PLACEHOLDER__3.5.0/boogg/boogg-min.js') >= 0, 'Group combo URL should be included in the result');
Assert.isTrue(out.js.indexOf('http://yui.yahooapis.com/combo?3.5.0/cookie/cookie-min.js&3.5.0/dom-screen/dom-screen-min.js') >= 0, 'Default YUI combo URL should be included in the result');
},
'test inheritted maxURLLength with groups if comboBase is the same': function () {
var loader = new testY.Loader({
combine: true,
groups: {
testGroup: {
// Set an incredibly low maxURLLength - this should be ignored.
maxURLLength: 10,
modules: {
foogg: {
requires: []
},
boogg: {
requires: []
}
}
}
},
require: ['foogg', 'boogg', 'cookie', 'dom-screen']
});
var out = loader.resolve(true);
Assert.areSame(1, out.js.length, 'Loader generated a single URL for a single comboBase');
},
test_resolve_maxurl_length: function() {
var loader = new testY.Loader({
maxURLLength: 1024,
Expand Down