Static handler and combo consolidation #697

merged 8 commits into from Nov 6, 2012


None yet

2 participants

  • combo is now part of the static handler middleware
  • all static files are now combo-ables
  • cache is now enable at the file level
  • experimental combo-handler middleware removed

One of the things is happening at the moment is that all requests are treated as potential combo or static requests, this is clearly an overhead. In theory, we should be able to leverage appConfig.staticHandling.prefix to validate that the url is a potential static url to avoid doing extra operations everytime, the problem is that prefix if a default value of the store and not a default value of the configs.

@drewfish drewfish commented on the diff Nov 6, 2012
+ base: "/static/combo?",
+ comboBase: "/static/combo?",
+ root: ""
+ }, ((appConfig.yui && appConfig.yui.config) || {}));
+ // used to find the the modules in YUI itself
+ Y.use('loader');
+ modules = (new Y.Loader(Y.config)).moduleInfo;
+ for (name in modules) {
+ if (modules.hasOwnProperty(name)) {
+ // faking a RS object for the sake of simplicity
+ fullpath = libpath.join(__dirname,
+ '../../../node_modules/yui', modules[name].path);
+ mimetype = libmime.lookup(fullpath);
+ charset = libmime.charsets.lookup(mimetype);
drewfish Nov 6, 2012 Yahoo Inc. member

Since we know these YUI modules are javascript files, can we hardcode the MIME type and charset?

caridy Nov 6, 2012

they could be CSS since YUI uses yui modules meta for css as well.

Yahoo Inc. member

I see this in the travis output for the unit tests:

[ERROR] LibManager - Error: ENOENT, no such file or directory '../../../../../lib/app/middleware/mojito-combo-handler.js'
1668Loading dependency: ../../../../base/mojito-test.js
1669Loading dependency: /home/travis/builds/yahoo/mojito/node_modules/yahoo-arrow/lib/common/yui-arrow.js
1670Executing test: ./test-combo-handler.js
1673    throw err;
1674          ^
1675Error: Cannot find module '/home/travis/builds/yahoo/mojito/tests/unit/lib/app/middleware/test-combo-handler.js'
1676    at Function.Module._resolveFilename (module.js:338:15)
1677    at Function.Module._load (module.js:280:25)
1678    at Module.require (module.js:362:17)
1679    at require (module.js:378:17)
1680    at Object.ARROW.onSeeded (/home/travis/builds/yahoo/mojito/node_modules/yahoo-arrow/nodejs/node.js:93:9)
1681    at onYUIAvailable (/home/travis/builds/yahoo/mojito/node_modules/yahoo-arrow/lib/client/yuitest-seed.js:133:15)
1682    at /home/travis/builds/yahoo/mojito/node_modules/yahoo-arrow/lib/client/yuitest-seed.js:140:13
1683    at Object.<anonymous> (/home/travis/builds/yahoo/mojito/node_modules/yahoo-arrow/lib/client/yuitest-seed.js:152:3)
1684    at Module._compile (module.js:449:26)
1685    at Module._extensions..js (module.js:467:10)
Yahoo Inc. member

It looks like test-handler-static.js has a negative combo test (in "bad or missing files") but it doesn't appear to have a positive combo test.

@drewfish drewfish commented on the diff Nov 6, 2012
var appConfig = store.getStaticAppConfig(),
options = appConfig.staticHandling || {},
cache = options.cache,
maxAge = options.maxAge,
- urls = store.getAllURLResources();
+ staticRess,
+ moduleRess,
+ yuiRess;
+ logger = globalLogger;
+ moduleRess = getAppModuleResources(store);
+ yuiRess = getYUIModuleResources(appConfig);
drewfish Nov 6, 2012 Yahoo Inc. member

This appears to be mixing apples and oranges. The getYUIModuleResources() method looks like it's returning resources, but it really is returning YUI loader config info. So I think it should either return resources (real or faked), or yuiRess, getYUIModuleResources(), and the res of res: yuiRess[module] (later on) should be renamed.

caridy Nov 6, 2012

Hmm, didn't quite get what you mean. Both methods return a object in the same form of {moduleName: res}, in the case of yuiRess, they are fake "resources".

drewfish Nov 6, 2012 Yahoo Inc. member

Oh sorry, I didn't see that getYUIModuleResources() was returning fake resources. Yeah, it's fine as is.


@drewfish I remember we removed a file from "middleware/test-handler-static.js" as some point.

Yahoo Inc. member

In the "valid combo url" test, the comment "combining an existing file with an invalid one should trigger 400" is wrong I think.

Otherwise, +1.

@caridy caridy merged commit 8345691 into yahoo:develop-perf Nov 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment