Sweetandsour2 #874

Merged
merged 10 commits into from Jan 15, 2013

Projects

None yet

2 participants

@bthaeler

This is a new hook system that replaces the embedded perf code. This will be the foundation for a new debugger interface.

@caridy

@bthaeler a couple of questions:

  • did you validate the unit tests? I remember when I put in place the perf there were few issues with the tests due to the fact that every script is using Y.mojito.hooks, which is undefined if it is not part of the testsuite mocks.

  • is the current output format of the perf still preserved? if not, we need to verify with @drewfish about this impact of that since we use it in the current profiler and potentially in our CI to monitor performance, but I'm not sure about that last part.

@caridy

According to @drewfish we are not using perf and profiler at all in our CI, so, feel free to break it, and even remove it in favor of the new profiler.

So, the only thing pending is the tests to pass.

@bthaeler

The existing perf markers are re-implemented using the hook system. So they should continue to work just fine.

It wasn't very clear to me how to run the unit tests. So I didn't. Looking at the Travis build output on github, it looks like some of the tests do complain. I can try and fix those. Is there a good current document explaining how to setup a test system for running the unit tests?

@caridy

Few pointers:

@bthaeler

I added some missing mojito-hooks dependency to places, and I added a stuff for the hook system to the test base. I think the unit tests are fine now.

@caridy caridy commented on the diff Jan 9, 2013
tests/base/mojito-test.js
@@ -26,6 +26,12 @@ YUI.add('mojito', function(Y, NAME) {
Y.namespace('mojito.addons');
Y.namespace('mojito.addons.ac');
Y.namespace('mojito.addons.viewEngines');
+ // HookSystem::StartBlock
+ Y.mojito.hooks = {
+ hook: function () {},
@caridy
caridy Jan 9, 2013

I think enableHookGroup is also missing from this. Even though only perf is calling it conditionally, I think we will have more scripts calling it, in which case we need a mock for testing.

@caridy
caridy Jan 11, 2013

Any update on this one?

@bthaeler
bthaeler Jan 11, 2013

yes, I added a stub function.

    enableHookGroup: function () {}
@caridy caridy commented on the diff Jan 9, 2013
lib/mojito.js
@@ -281,6 +281,13 @@ MojitoServer.prototype._configureAppInstance = function(app, options) {
outputHandler.setLogger({
log: Y.log
});
+ // HookSystem::StartBlock
+ if (!req.hook) {
+ req.hook = null;
@caridy
caridy Jan 9, 2013

What is the purpose of setting it to null when it is already a falsy value?

@bthaeler
bthaeler Jan 9, 2013

From my, and your performance tests, it is more efficient to test if a field is null vs undefined. IE 'if (x.y)' is more efficient if x.y = null vs x.y is just undefined.

I am just trying to make the hook system as low impact as possible.

@caridy
caridy Jan 11, 2013

ok, we can revisit this later on after the first merge.

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/mojito.js
@@ -281,6 +281,13 @@ MojitoServer.prototype._configureAppInstance = function(app, options) {
outputHandler.setLogger({
log: Y.log
});
+ // HookSystem::StartBlock
+ if (!req.hook) {
+ req.hook = null;
+ }
+ outputHandler.hook = req.hook;
+ command.instance.hook = req.hook;
@caridy
caridy Jan 9, 2013

Any particular reason for adding hook into the instance instead of command. In theory, an instance's action can only be executed if there is a command to be dispatched. In my mind, hook is more tied to command than it is to instance. In any case, you should have access to command from everywhere during the dispatch process, just like instance. In the server, this is just a technicallity, but in the client, that instance is persistent, and command is created every time an action is invoked.

@caridy caridy and 1 other commented on an outdated diff Jan 9, 2013
lib/app/autoload/mojito-client.client.js
@@ -629,6 +642,12 @@ YUI.add('mojito-client', function(Y, NAME) {
outputHandler = new Y.mojito.OutputHandler(viewId, cb, this);
+ // HookSystem::StartBlock
+ if (Y.mojito.hooks) {
+ outputHandler.hook = Y.mojito.hooks.client_ctx;
@caridy
caridy Jan 9, 2013

client_ctx sounds weird here, specially when you assign it to outputHandler.hook. Is client_ctx the persistent hook in the client side? If yes, can we rename it? We don't want to keep overloading the context term in mojito, we already use it for many things.

@bthaeler
bthaeler Jan 9, 2013

Originally this was for the clients. But this also works for people who want to enable a hook global, without associating this with a request. So how about 'global_hooks'.

Each callback will have its own unique this state per tool. This is stored in this object. So do we want 'global_hooks' or 'global_hooks_ctx'?

@caridy caridy commented on the diff Jan 9, 2013
lib/app/autoload/hooks.common.js
+ * @param {String} tool tool name for this hook
+ * @param {String} hook name of hook
+ * @param {Function} cb call back function
+ *
+ * Example:
+ * <pre>
+ * Y.mojito.hooks.registerHook('test_tool, 'test_hook', function (reg, data) {});
+ * </pre>
+ */
+ Y.mojito.hooks.registerHook = function (tool, hook, cb) {
+ if (!map_hook_tool[hook]) {
+ map_hook_tool[hook] = {};
+ }
+ map_hook_tool[hook][tool] = cb;
+
+ Y.mojito.hooks.hook = Y.mojito.hooks.real_hook;
@caridy
caridy Jan 9, 2013

Why is this happening here? This sounds like a leak-prompt structure, because any request can potentially change this. If the hook is something created per request, then it should be an inmutable object.

@bthaeler
bthaeler Jan 9, 2013

Not sure what your concern is here. Hooks are statically defined. Groups of hooks are enabled on a per request basis. What hooks people are interested in is static.

@caridy
caridy Jan 11, 2013

My question is why hook gets real_hook whenever a new hook is register? is this something to have "hook" defined only if there is at least one hook registered? I'm just curious about this statement.

@bthaeler
bthaeler Jan 11, 2013

Yes, the idea is that if you are not using any code that needs a hook, the hook system defaults to an empty function.

@caridy caridy and 1 other commented on an outdated diff Jan 9, 2013
lib/app/autoload/hooks.common.js
+ *
+ * @method enableHookGroup
+ * @param {Object} req Request object to enable hook on
+ * @param {String} tool Name of tool to enable
+ * @return {Object} this tools context used for its hook callbacks
+ *
+ * Example:
+ * <pre>
+ * Y.mojito.hooks.enableHookGroup(req, 'test_tool');
+ * </pre>
+ */
+ Y.mojito.hooks.enableHookGroup = function (req, tool) {
+ var ret = {};
+ // Y.log("enable tool: " + tool);
+
+ // if called in client, use single global context.
@caridy
caridy Jan 9, 2013

In my experience, trying to accomodate a component to be common when in reality the behavior of the client and server are very different will bring problems. I wonder if we should split this into server and client.

@bthaeler
bthaeler Jan 9, 2013

If I rename this to 'global_hooks' I think this resolved this.

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/app/autoload/hooks.common.js
+ * @method hook
+ * @param {String} hook_name
+ * @param {Object} hook context (from req.hook, or adapter.hook)
+ * @param {} hook specific data
+ *
+ * Example:
+ * <pre>
+ * Y.mojito.hooks.hook('test_hook', ctx, ...);
+ * </pre>
+ */
+ Y.mojito.hooks.hook = function () {
+ // Y.log("hooks disabled");
+ };
+
+ Y.mojito.hooks.real_hook = function (hook, ctx) {
+ // Y.log("in hook handler: " + hook);
@caridy
caridy Jan 9, 2013

Please, preserve all Y.log(msg, 'info or debug or error', NAME), instead of commenting them.

@caridy caridy commented on the diff Jan 9, 2013
lib/app/autoload/hooks.common.js
+ */
+
+
+/*jslint anon:true, sloppy:true*/
+/*global YUI*/
+
+/**
+ * The hook system provides a way for application or add on developers to access the inner working of mojito.
+ * There are two steps to using an hook. First, an addon needs to register an interest in a hook by calling
+ * registerHook with the name of the hook and a callback function. The second step involves enabling a hook
+ * your addon to recieve hooks. You need to call the enableHookGroup with the name of your addon.
+ *
+ * List of hooks
+ *
+ * <ul>
+ * <li>Y.mojito.hooks.hook('adapterBuffer', hook_ctx, 'start', this);
@caridy
caridy Jan 9, 2013

Why do we have these docs here? this is the hook utility, it will be difficult to keep this in sync with all the hooks defined across the mojito or the app. We can create a tool to generate such as list, or we can document them in a form of YUI events on site, rather than here.

@caridy
caridy Jan 11, 2013

any update on this one?

@bthaeler
bthaeler Jan 11, 2013

Sounds like you are proposing two options. One is to create a script that runs at build time to generate this list, and the second one is using some kind of YUI event to do this? I don't now what would be better. Is this something we want to solve now or latter? If you feel strongly about this, I can try and figure something out.

@caridy caridy and 1 other commented on an outdated diff Jan 9, 2013
lib/app/addons/view-engines/hb.server.js
@@ -39,26 +39,32 @@ YUI.add('mojito-hb', function(Y, NAME) {
*/
render: function (data, mojitType, tmpl, adapter, meta, more) {
var cacheTemplates = meta && meta.view && meta.view.cacheTemplates,
- perf = Y.mojito.perf.timeline('mojito', 'hb:render',
- 'time to render a template', tmpl),
- handler = function (err, obj) {
- var output;
+ handler;
@caridy
caridy Jan 9, 2013

Any particular reason for moving handler's function definition down rather than in the var statement?

@bthaeler
bthaeler Jan 9, 2013

No, i will fix.

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/app/addons/ac/composite.common.js
@@ -204,8 +209,10 @@ callback({
var ac = this.ac,
buffer = {},
content = {},
- meta = {},
- perf;
+ // HookSystem::StartBlock
+ self = this,
@caridy
caridy Jan 9, 2013

my convention, we use my = this instead of self = this. Don't ask my why :)

And by the way, don't need to define the comments for that line, it is ok to have my defined even when u don't use it later on.

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/app/addons/ac/composite.common.js
this.buffer = buffer;
this.id = id;
this.callback = callback;
this.__meta = [];
- this.__perf = Y.mojito.perf.timeline(NAME, 'child', 'the whole child', id);
+ // HookSystem::StartBlock
+ this.__hook_ctx = hook_ctx;
@caridy
caridy Jan 9, 2013

Can you elaborate more about this __hook_ctx very private reference?

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/app/addons/ac/composite.common.js
@@ -14,19 +14,24 @@
*/
YUI.add('mojito-composite-addon', function(Y, NAME) {
- function AdapterBuffer(buffer, id, callback) {
+ function AdapterBuffer(buffer, id, callback, hook_ctx) {
@caridy
caridy Jan 9, 2013

This is the only part that I really don't like. Extending the signature of the class to add the hook reference after the callback is not pretty. Let's try to find another way to keep that reference around.

@caridy caridy commented on an outdated diff Jan 9, 2013
lib/app/addons/ac/composite.common.js
@@ -335,7 +346,7 @@ callback({
};
childAdapter = new AdapterBuffer(buffer, childName,
- callback);
+ callback, this.adapter.hook);
@caridy
caridy Jan 9, 2013

same as above, we should not do this here. Let's find another way for the adapter to pull a reference of the current hook.

@caridy

@bthaeler that's all for now. Let's work together to accomodate those details so we can finally merge it.

@caridy

+1

@bthaeler I think we are ready to merge it. Since we have a release coming up on monday, I will hold this until after that.

/cc @jenny

@bthaeler

Any idea when you will do the merge for this pull request?

@caridy

today or tomorrow! 0.5.2 is out!

@caridy caridy merged commit 15e22ca into yahoo:develop Jan 15, 2013

1 check failed

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