[wip] hooks to patch pre/post initialized YUI instance #1590

Merged
merged 9 commits into from Mar 17, 2014

Conversation

Projects
None yet
4 participants
@ekashida
Contributor

ekashida commented Jan 28, 2014

adding a hook inside the loader constructor which allows the monkey-patching of loader and the yui instance

ekashida added some commits Jan 28, 2014

move patch hook to loader instantiation time
This hook needs to happen earlier if we want to capture all the work
that loader does during instantiation.
@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Jan 30, 2014

Owner

Let's more into a more simple structure using hooks (as flags) rather than consuming configuration with a rigid structure. Something like:

Y.config.hookForSomething && Y.config.hookForSomething(Y, loader);
Owner

caridy commented Jan 30, 2014

Let's more into a more simple structure using hooks (as flags) rather than consuming configuration with a rigid structure. Something like:

Y.config.hookForSomething && Y.config.hookForSomething(Y, loader);
src/loader/js/loader.js
+ // Hook to patch loader
+ if (o.hookForLoader) {
+ o.hookForLoader(Y, self);
+ }

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Feb 7, 2014

Owner

What's the intention here? AOP, do-before? Also is the return value significant?

Do we need this because we cannot replace the Loader constructor before it's used when creating a new YUI instance?

I also feel strongly that we need a better name. This name seems to imply that you're providing some hook for loader to use.

@ericf

ericf Feb 7, 2014

Owner

What's the intention here? AOP, do-before? Also is the return value significant?

Do we need this because we cannot replace the Loader constructor before it's used when creating a new YUI instance?

I also feel strongly that we need a better name. This name seems to imply that you're providing some hook for loader to use.

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Feb 7, 2014

Owner

some sort of AOP, but really this is just a dynamic extension of loader. Return value is not relevant since this is a constructor.

Yes, we need to patch the loader before anything else happens.

Yes, better name, and better structure is needed, I will propose a trully dynamic extension, e.g.:

if (o.doBeforeLoader) {
     o.doBeforeLoader.apply(this, arguments);
}

I don't think we need to pass Y into the method since there is a property of this called context that is set as part of the class definition.

@caridy

caridy Feb 7, 2014

Owner

some sort of AOP, but really this is just a dynamic extension of loader. Return value is not relevant since this is a constructor.

Yes, we need to patch the loader before anything else happens.

Yes, better name, and better structure is needed, I will propose a trully dynamic extension, e.g.:

if (o.doBeforeLoader) {
     o.doBeforeLoader.apply(this, arguments);
}

I don't think we need to pass Y into the method since there is a property of this called context that is set as part of the class definition.

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 12, 2014

Owner

tests are missing...

Owner

caridy commented Mar 12, 2014

tests are missing...

@yahoocla

This comment has been minimized.

Show comment Hide comment
@yahoocla

yahoocla Mar 12, 2014

CLA is valid!

CLA is valid!

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 13, 2014

Owner

LGTM (can you investigate the test failure?)

Maybe we should provide an example in the API docs, maybe as part loader class description.

Owner

caridy commented Mar 13, 2014

LGTM (can you investigate the test failure?)

Maybe we should provide an example in the API docs, maybe as part loader class description.

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 13, 2014

Owner

👍

Owner

caridy commented Mar 13, 2014

👍

@ekashida ekashida merged commit f395b31 into yui:dev-master Mar 17, 2014

1 check passed

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