Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for implicit args in init methods #540

Open
mjclemente opened this issue Dec 16, 2021 · 2 comments
Open

Support for implicit args in init methods #540

mjclemente opened this issue Dec 16, 2021 · 2 comments

Comments

@mjclemente
Copy link
Contributor

I posted about this in Slack, and thought it worth logging here, for discussion.

I ran into an interesting issue when trying to use Eric Peterson's Hyper library with FW/1. I set the defaults, as laid out in the docs, in my Loadlistener, like this:

.declare( 'apiClient' ).instanceOf( 'hyper.models.HyperBuilder' ).asSingleton().withOverrides(
  {
    baseUrl = 'https://thehost.com/api/v1/whatever',
    headers = {
      "Token" = "xxx-xxx-xxx"
    },
    timeout = 12
  }
).done()

While the overrides get picked up, the ultimately don't get applied to the HyperBuilder cfc, because it doesn't explicit list its args or properties. It just looks for them in the args scope. The init method looks like this:

function init() {
this.defaults = new Hyper.models.HyperRequest();
for ( var key in arguments ) {
  invoke(
    this.defaults,
    "set#key#",
    { 1 : arguments[ key ] }
  );
}
return this;
}

If I modify the init method to explicitly include the args (baseUrl, headers, timeout), then it works as expected.

Basically, I expected FW/1 to pass my overrides into the init method, whether or not they were defined there explicitly, and that's not happening.

I did find that I can generate the behavior I'm looking for with a small change to ioc.cfc. If I add the following around line 879:

if( isEmpty( info.metadata.constructor ) ){
  args = overrides;
} 

Basically, if there's no constructor args, but there are overrides, use them when creating the bean.

Does this seem like a change worth making?

Finally, I should note that some really helpful suggestions were made for working around this limitation. I ultimately did something like this:

di1.declare( "apiClient" )
  .asValue( new hyper.models.HyperBuilder(   
    baseUrl = 'https://thehost.com/api/v1/whatever',
    headers = {
            "Token" = "xxx-xxx-xxx"
    },
    timeout = 12
 ) );
@lramirez925
Copy link

I definitely agree that if you provide overrides then those should be passed to the init no matter what.

The one thing I don't know about is wether we should only do that when constructor is empty (your implementation). I feel if we pass overrides they should be sent to the init no matter what. I haven't tested and had only a brief look over the code, but it should probably always merge the overrides into the arg struct, giving the writing dev the most flexibility. But that's just my opinion there.

@tonyjunkes
Copy link
Member

I like the idea. I do wonder if this should be something that is applied every time there are no constructor arguments, which I agree with, or should this be driven by a boolean flag passed in withOverrides to represent allowing values to be passed without any matching arguments.

With the latter, you could then have syntax like the following:

di1.declare( "MyBean" ).instanceOf( "some.cfc" ).withOverrides( { arg: value }, true );

This would suggest adding a second argument to the withOverrides function expression in the declare function of ioc.cfc and passing it along with the overrides. Then in resolveBeanCreate check for no constructor arguments (as originally mentioned), but also check for the flag to be set before setting the overrides.

When this was mentioned on Slack, I started playing around with the idea and have working code with the flag approach applied. It could easily be modified to always apply the args too. I'm just not entirely sure about any caveats or expansions beyond what's been mentioned already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants