Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Sweetandsour2 #874

Merged
merged 10 commits into from Jan 15, 2013

Conversation

Projects
None yet
2 participants
Contributor

bthaeler commented Jan 4, 2013

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

bthaeler added some commits Dec 20, 2012

build hook system into mojito
remove perf markers, and replace with hooks.
build hook system into mojito
remove perf markers, and replace with hooks.
Merge branch 'develop' of git://github.com/yahoo/mojito into sweetand…
…sour2

Conflicts:
	lib/app/autoload/store.server.js
Collaborator

caridy commented Jan 8, 2013

@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.
Collaborator

caridy commented Jan 8, 2013

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.

Contributor

bthaeler commented Jan 8, 2013

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?

Collaborator

caridy commented Jan 8, 2013

Few pointers:

added some missing hook dependencys.
added stub to test harnas
Contributor

bthaeler commented Jan 9, 2013

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

Collaborator

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

Collaborator

Any update on this one?

@bthaeler

bthaeler Jan 11, 2013

Contributor

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

Collaborator

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

@bthaeler

bthaeler Jan 9, 2013

Contributor

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

Collaborator

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

Collaborator

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

Collaborator

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

Contributor

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

Collaborator

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

Contributor

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

Collaborator

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

Contributor

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 Jan 11, 2013

Collaborator

ok

@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

Collaborator

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

Contributor

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

Collaborator

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

Collaborator

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

Collaborator

any update on this one?

@bthaeler

bthaeler Jan 11, 2013

Contributor

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

Collaborator

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

@bthaeler

bthaeler Jan 9, 2013

Contributor

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

Collaborator

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

Collaborator

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

Collaborator

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

Collaborator

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.

Collaborator

caridy commented Jan 9, 2013

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

Collaborator

caridy commented Jan 11, 2013

+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

Contributor

bthaeler commented Jan 15, 2013

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

Collaborator

caridy commented Jan 15, 2013

today or tomorrow! 0.5.2 is out!

caridy added a commit that referenced this pull request Jan 15, 2013

Merge pull request #874 from bthaeler/sweetandsour2
Adding support for junebug:

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

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

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