Tweak factory function parameters and scope about deftjs HOT 15 CLOSED

I am currently working on this.

In preparation I have ported Deft.ioc.Injector specs from Jasmine to Mocha. This is based on the promises_aplus branch as master does not appear to have Mocha tests.

See https://github.com/superstructor/DeftJS/blob/feature/35/test/coffee/Deft/ioc/Injector.coffee

Next I will port Deft.mixin.Injectable specs.

Each spec file port will be in an independent commit, and so will the fix for this issue (3 commits total).

Do you want 3 pull requests with 1 commit each or just 1 with the 3 commits ?

Should I wait until promises_aplus is merged into master to open a pull request since I am based off that branch ? Obviously I'll rebase as appropriate.

Cheers

from deftjs.

Discussed with @johnyanarella

Will do

• 1 pull request with 3 commits (Injector tests, Injectable tests, and the change for this issue)
• wait until promises_aplus branch is merged to make a pull request

Cheers

from deftjs.

Deft.mixin.Injectable specs ported to Mocha are here https://github.com/superstructor/DeftJS/blob/feature/35/test/coffee/Deft/mixin/Injectable.coffee

from deftjs.

Changes for this issue with tests are done.

Just have two failing tests, that from debugging I don't believe are a bug in the code but a bug in the tests themselves.

The factory functions are executed in the targetInstance scope and passed all of the constructor arguments as parameters for automatic injection.

In addition to the original requirements of the ticket I pass targetInstance.getInitialConfig() to the factory function as a parameter when called via manual injection with .inject() on an existing targetInstance. This is as close to the automatic injection parameters I can get without writing a class pre-processor to capture initial constructor arguments and stash them somewhere for later "just in case" you manually inject something. My view is that would be an unnecessary performance and memory burden.

As soon as promises_aplus is merged I can make a pull request.

Cheers

from deftjs.

https://github.com/superstructor/DeftJS/tree/feature/35

from deftjs.

Excellent! This sounds great.

promises_aplus was just merged into master.

from deftjs.

I see your point about the complexity and overhead of capturing the initial constructor arguments. Agreed, that would be a very bad idea from a performance and memory perspective.

So, digging into Ext.Basefor both Ext JS and Sencha Touch, it turns out that instance.getInitialConfig() returns the merged configuration object (i.e. this.config), rather than the original configuration object that was passed to the constructor. However, Ext JS 4.1+ and Sencha Touch 2.0+ all have an instance.initialConfig property that does contain the original configuration object that was passed to the constructor.

Swapping out the use of targetInstance.getInitialConfig() in Deft.ioc.Injector for targetInstance.initialConfig makes those tests pass as expected.

However, the instance.initialConfig property is not available in Ext JS 4.0.7. @brian428 At this point - do we care about supporting 4.0.7? The documentation already refers to 4.1 as the oldest supported version.

from deftjs.

Awaiting feedback re: use of instance.initialConfig.

from deftjs.

It looks like instance.initialConfig is undocumented, so it could potentially disappear in future releases of Ext JS or Sencha Touch.

We could potentially solve this with an override for Ext.create() that mimics how instance.initialConfig is initialized, but with an instance property of our own (ex. instance.$initialConfig). Since it would be a reference to the same object instance.initialConfig is already holding on to, I'm not too worried about the memory impact.

from deftjs.

Crud. instance.initialConfig is only populated for classes that call initConfig(). (And Sencha's own classes don't consistently adhere to that convention.)

from deftjs.

John, we already have some unit tests that fail in versions prior to 4.1. I have no problem dropping support for it. Knowing that we would at some point was part of the reason I gave 4.1 as the "officially" supported version in the docs. No one's complained yet....I can't imagine there's many people left using 4.0.x.

from deftjs.

For the scenario where a factory function is fulfilling a dependency via a manual call to Deft.Injector.inject() on an already instantiated class instance rather than as part of the instance initialization via inject:, I'm inclined to omit the config parameter argument entirely. As @superstructor already noted, we can't send the factory function the rest of the original constructor parameters anyway.

We're talking about an advanced edge case here. A factory function being used in that manner would more likely inspect the target instance itself, rather than needing the original parameters that were passed to its constructor.

I'm also having second thoughts about breaking any existing factory functions in use today by changing the scope that factory function is executed in and no longer passing the instance as the first parameter.

At this point, I think I'd rather see us:

• always execute the factory function in window (or maybe Deft.Injector for convenient Deft.Injector.resolve() calls?) scope
  • this being the target instance seems really odd to me when seen in the context of IoC container configuration block where the factory function is declared
• pass the instance followed by the constructor parameters when the factory function is called during instance initialization via inject:
• only pass the instance when the factory function is called when manually executing Deft.Injector.inject() on an already instantiated class instance.

This gives some degree of consistency between the two scenarios - both are passed the target instance, and the constructor arguments are essentially optional parameters.

Thoughts?

from deftjs.

👍 Agreed.

from deftjs.

Pull request made with requested changes.

Cheers

from deftjs.

Excellent work. Thanks again!

from deftjs.