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

Resolving constructor arguments for derived class includes bound arguments from base classes #1565

Closed
mgabeler-lee-6rs opened this issue Jul 26, 2018 · 6 comments
Assignees

Comments

@mgabeler-lee-6rs
Copy link
Contributor

Description / Steps to reproduce

When a class is being instantiated by a binding, the computed (injected) arguments for the constructor will end up including all such arguments for any base class constructors too.

The crux of this issue seems to be at https://github.com/strongloop/loopback-next/blob/master/packages/context/src/resolver.ts#L57 and https://github.com/strongloop/loopback-next/blob/master/packages/context/src/resolver.ts#L156 -- resolveInjectedArguments does not pass options down to describeInjectedArguments and thus to MetadataInspector.getAllParameterMetadata to tell it to only include 'own' parameters.

As a result, you cannot really change constructor arguments in a derived class, or from the alternate perspective, you cannot put @inject on any constructor arguments of a base class.

Current Behavior

class Base {
  constructor(@inject(/*...*/) foo: Something) {}
}
class Derived {
  constructor() { super(/*alternate source of a Something*/) }
}

This will fail to create an instance of Derived for two reasons:

  1. It will look for the binding for the base class' @inject. If this binding is not present, it will throw an error, even though it's not needed.
  2. If it does find a value for the @inject it will try to pass it to Derived's constructor, which is wrong. It would get even more wrong if Derived had an argument of its own -- then it would be passing a completely wrong value.

Expected Behavior

Injection of constructor arguments should only be for the actual constructor being invoked, and should not include arguments for base class constructors.

@shimks
Copy link
Contributor

shimks commented Aug 7, 2018

@raymondfeng I'm not too familiar with the intended behavior of @inject in this scenario. Could you please comment?

@raymondfeng
Copy link
Contributor

@mgabeler-lee-6rs I'll investigate.

@raymondfeng
Copy link
Contributor

This is tricky. We have two cases:

  1. A sub class with an explicit constructor
class SubClass extends BaseClass {
  constructor() {
    super('sub');
  }
}
  1. A sub class without an explicit constructor
class SubClass extends BaseClass {
}

For case 1, we want to ignore injection from the base constructor. But for case 2, we would like to honor injection from the base constructor.

Now the challenge is to differentiate the two cases and that seems to be hard. Any ideas?

@mgabeler-lee-6rs
Copy link
Contributor Author

If nothing else, perhaps a decorator on the derived class constructor describing the policy for argument injections from the base class(es)? At least all or none would be wanted there, with the default policy being all so the no explicit constructor case works out of the box. The possible scenarios for "some" seem quite messy and complicated, though, and perhaps in that scenario the derived class should just be required to repeat the subset of the bindings it wants to use? Would need to handle that recursively after a fashion -- e.g. if BaseBase declares injections, Base says not to inherit them and instead replaces them, and then Sub says to inherit, need to make sure it inherits the effective injections from Base, not all the injections from the full hierarchy.

@raymondfeng
Copy link
Contributor

@mgabeler-lee-6rs Can you verify #1596?

@mgabeler-lee-6rs
Copy link
Contributor Author

I'll try to do that next week, yes.

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

No branches or pull requests

4 participants