upgrading to YUI 3.7.2 to facilitate the use of new loader #608

Merged
merged 6 commits into from Oct 10, 2012

Conversation

Projects
None yet
2 participants
Collaborator

caridy commented Oct 10, 2012

  • improving loader integration
  • adding some guarding on combo handler.
  • formalizing extensions and content-type.
  • supporting ylinux/yiv
  • adding support for css on the combo handler
- module = module.replace(/\-(min|debug)$/, '');
+ // something like foo/bar-min.js should become just "bar"
+ module = libpath.basename(files[i]).
+ replace(/\.(js|css)$/, '').
@drewfish

drewfish Oct 10, 2012

Contributor

Using libpath.basename(files[i], libpath.extname(files[i])) instead of the replace(/\.(js|css)$/, '') is a little more correct/robust, but might not be worth the effort.

@caridy

caridy Oct 10, 2012

Collaborator

I added the extension explicitly to trigger the error if they try to access something else, but later on I added a verification of the extension, so I can use this structure now. Should be ready in the next commit.

+ // YIV might be messing around with the querystring params
+ // so we need to revert back to /
+ // TODO: this might not work in Windows
+ files = url.query.replace(/\=/g, '').replace(/\%2F/g, '/').split('&');
@drewfish

drewfish Oct 10, 2012

Contributor

What does \= match? I didn't think that = was a special token in regex, and thus doesn't need escaping. Ditto with \%2F, I didn't think that % was special.

@caridy

caridy Oct 10, 2012

Collaborator

lol, I'm not very good at regex in general, so I escape anything that it suspicious. Feel free to clean that up.

@drewfish

drewfish Oct 10, 2012

Contributor

http://nodemanual.org/latest/js_doc/RegExp.html indicates that neither = nor % are special, so I think the regexes should be /=/g and /%2F/g instead.

Contributor

drewfish commented Oct 10, 2012

Besides two small comments, +1.

if (module === 'loader-app-base') {
result[i] = {
fullpath: module,
- content: "YUI.applyConfig({modules:" + appMetaData + "});"
+ content: 'YUI.add("loader",function(Y){' +
+ 'YUI.Env[Y.version].modules=YUI.Env[Y.version].modules||' + appMetaData + ';' +
@drewfish

drewfish Oct 10, 2012

Contributor

This code seems a little strange -- it seems like mojito has too much knowledge of how YUI works. Is there a YUI API call we can make instead (loader.addGroup() or something like that) ?

@caridy

caridy Oct 10, 2012

Collaborator

In theory, it should use YUI.applyConfig(), but there is a remaning bug in yui that prevents us from using the config API. Instead, we need to plug them directly into the main object. Dav will have this fix for the next release.

@drewfish

drewfish Oct 10, 2012

Contributor

Please add that information to a comment. We really should circle back and do it the right way (once the right way is available).

};
} else if (module === 'loader-app-full') {
result[i] = {
fullpath: module,
- content: "YUI.applyConfig({modules:" + appResolvedMetaData + "});"
+ content: 'YUI.add("loader",function(Y){' +
+ 'YUI.Env[Y.version].modules=YUI.Env[Y.version].modules||' + appResolvedMetaData + ';' +
@drewfish

drewfish Oct 10, 2012

Contributor

Ditto previous comment.

};
} else if (urls[module]) {
result[i] = {
fullpath: urls[module],
content: ''
};
- } else if ((module.indexOf('..') === -1) && existsSync(yui)) {
+ } else if ((files[i].indexOf('..') === -1) && existsSync(yui)) {
@drewfish

drewfish Oct 10, 2012

Contributor

Is existsSync() being called during runtime? That might have bad performance implications.

@caridy

caridy Oct 10, 2012

Collaborator

In theory, in production, we will use the YCS combo rather than the app combo. But I do agree we should make this more performance, maybe creating a hash in memory for all YUI modules or just not dealing with YUI modules at all. Let me think about this.

Contributor

drewfish commented Oct 10, 2012

+1

caridy added a commit that referenced this pull request Oct 10, 2012

Merge pull request #608 from caridy/loader-meta
upgrading to YUI 3.7.2 to facilitate the use of new loader

- upgrade to YUI 3.7.2
- improving loader integration
- adding some guarding on combo handler.
- formalizing extensions and content-type in combo.
- supporting ylinux/yiv in combo
- adding support for css on the combo handler

@caridy caridy merged commit 4164f22 into YahooArchive:develop-perf Oct 10, 2012

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment