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

V6 binding abstractions #1336

Draft
wants to merge 36 commits into
base: v6-binding-abstractions
Choose a base branch
from

Conversation

tonyhallett
Copy link
Contributor

@tonyhallett tonyhallett commented May 5, 2021

Description

As per #1327.

Binding no longer has properties for each "to" syntax call and activated and cache have also been removed.
Binding now has a value provider that provides what is to be resolved.
Binding now has the scope property of type Scope, one of the 5 different scopes that include the new inRootRequestScope and inCustomScope.

inRootRequestScope()
This works similarly to request scope but applies to all container.get calls that occur during the 'root' container.get. This allows scope to be shared across toDynamicValue/toFactory

container.bind<Weapon>(TYPE.DynamicWeapon).toDynamicValue((context) => {
    return context.container.get<Weapon>(TYPE.Weapon);
});

This is facilitated by Context.parentContext, which is managed by Container._planAndResolve, and the new RootRequestScope that stores in the rootRequestScope of the root Request object.

Related Issue

#1143

Motivation and Context

As soon as you use toDynamicValue/toFactory you cannot resolve a previously resolved value unless it is singleton scoped. With this code change there is now the concept of a root request which can be scoped to.

How Has This Been Tested?

Thoroughly.

Types of changes

  • Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the changelog.

@notaphplover
I will review this pull request tomorrow as I would like to discuss some aspects of it.

Copy link
Contributor Author

@tonyhallett tonyhallett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow file is set up for the master branch only.

Constructor: interfaces.BindingType;
DynamicValue: interfaces.BindingType;
Factory: interfaces.BindingType;
Function: interfaces.BindingType;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Determined that Factory BindingTypeEnum is unnecessary. Opinions please.

}

export interface NotConfiguredScope extends Scope<never>,Clonable<NotConfiguredScope>{
type:"NotConfigured",
Copy link
Contributor Author

@tonyhallett tonyhallett May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotConfiguredScope and NotConfiguredValueProvider could be renamed InvalidScope/InvalidValueProvider with type Invalid to adhere to previous BindingType/Enum

@@ -1,30 +1,44 @@
import { BindingScopeEnum } from "../constants/literal_types";
import { interfaces } from "../interfaces/interfaces";
import { BindingWhenOnSyntax } from "./binding_when_on_syntax";
import { BindingScopeScopeFactory as BindingScopeScopeFactoryInterface } from "../scope/binding-scope-scope-factory-interface";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to change to IBindingScopeScopeFactory ? Did not want to add to interfaces.ts internal interfaces.


class BindingInSyntax<T> implements interfaces.BindingInSyntax<T> {

private _binding: interfaces.Binding<T>;

public bindingScopeScopeFactoryInterface:BindingScopeScopeFactoryInterface<T> = new BindingScopeScopeFactory<T>()
Copy link
Contributor Author

@tonyhallett tonyhallett May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume ok for public properties that can use for testing, given that is not on the interface ?

scope = BindingScopeEnum.Singleton;
}
this._binding.scope = this.bindingScopeScopeFactory.get(scope);
return new BindingInWhenOnSyntax<T>(this._binding);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume ok to return this for methods that return interfaces.BindingWhenOnSyntax

serviceIdentifier: ServiceIdentifier<TActivated>;
constraint: ConstraintFunction;
dynamicValue: DynamicValue<TActivated> | null;
Copy link
Contributor Author

@tonyhallett tonyhallett May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dyamicValue, implementationType, factory, provider, cache ( in terms of constant value ) are now valueFrom on the ValueProvider

As per activated and cache do we want utility functions ?

serviceIdentifier: ServiceIdentifier<TActivated>;
constraint: ConstraintFunction;
dynamicValue: DynamicValue<TActivated> | null;
scope: BindingScope;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope now Scope which has type BindingScope

serviceIdentifier: ServiceIdentifier<TActivated>;
constraint: ConstraintFunction;
dynamicValue: DynamicValue<TActivated> | null;
scope: BindingScope;
type: BindingType;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is ValueProvider.type with the omission of Function

@@ -106,7 +110,8 @@ class Container implements interfaces.Container {
this._deactivations = new Lookup<interfaces.BindingDeactivation<any>>();
this.parent = null;
this._metadataReader = new MetadataReader();
this._moduleActivationStore = new ModuleActivationStore()
this._moduleActivationStore = new ModuleActivationStore();
this._contextStack = new ContextStack(this)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ContextStack provides the context hierarchy in conjunction with planAndResolve.
What I have not done is deal with snapshot and restore. The ContextStack uses the Container.options to stay current.
We currently do not snapshot and restore container options which seems incorerct to me.

Thoughts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also has not been tested as a unit.

}

private _determineRequiresContextHierarchy(): void {
let requiresContextHierarchyOption: interfaces.ContextHierarchyOption = ContextHierarchyOptionEnum.Allow;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults to Allow. I wanted it to be IfBindedInCustomOrRootRequestScope but the performance test
it("Should be able to register 10K bindings in less than 1 ms", () =>
failed as went to 3s.

Thoughts on default ?

@@ -116,7 +115,7 @@ function _postConstruct<T>(constr: interfaces.Newable<T>, instance: T): void | P
}

function _validateInstanceResolution(binding: interfaces.Binding<unknown>, constr: interfaces.Newable<unknown>): void {
if (binding.scope === "Transient") {
if (binding.scope.type === ConfigurableBindingScopeEnum.Transient) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed to Singleton #1335

this._binding.implementationType = constructor as any;
this._binding.scope = BindingScopeEnum.Singleton;
return new BindingWhenOnSyntax<T>(this._binding);
//tbd change generics
Copy link
Contributor Author

@tonyhallett tonyhallett May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//tbd change generics
Mentioned in #1319 (comment)

expect(bindingInWhenOn).to.be.instanceOf(BindingInWhenOnSyntax);
expect(bindingInWhenOn._binding === binding).to.equal(true);
}
expectBindingInWhenOnSyntax(bindingToSyntax.to(Sid));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was absent from original code. Other "to" methods can be added....

return resolved;
}
_getRootContextRootRequestStore(context:interfaces.Context):interfaces.RequestScope {
while(context.parentContext !== undefined){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add this code as helper methods to Context ?

@tonyhallett
Copy link
Contributor Author

@inversify/maintainers
Thoughts on this v6 change please

@PodaruDragos
Copy link
Contributor

@tonyhallett I'm not against this. I do think the code looks better this way, but maybe users like .to().blabla syntax.
From my point of view this change is ok, but I do want to know the opinion of @dcavanagh and @notaphplover.

import { interfaces } from "../interfaces/interfaces";

export class RootRequestScope<T> implements interfaces.RootRequestScope<T>{
type: "RootRequest" = "RootRequest";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we maybe get this from an Enum ? same for the others ?

type:"Instance";
}

export interface DynamicValueProvider<TActivated> extends
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. idk having strings like this is scary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your position.

From before.
The BindingType is a union of strings.
The BindingTypeEnum interface has been written so that it is possible for the literal to provide the same BindingType for all members

//interfaces
    export type BindingType = "ConstantValue" | "Constructor" | "DynamicValue" | "Factory" |
        "Function" | "Instance" | "Invalid" | "Provider";

    export interface BindingTypeEnum {
        ConstantValue: interfaces.BindingType;
        Constructor: interfaces.BindingType;
        DynamicValue: interfaces.BindingType;
        Factory: interfaces.BindingType;
        Function: interfaces.BindingType;
        Instance: interfaces.BindingType;
        Invalid: interfaces.BindingType;
        Provider: interfaces.BindingType;
    }
//literal-types
const BindingTypeEnum: interfaces.BindingTypeEnum = {
    ConstantValue: "ConstantValue",
    Constructor: "Constructor",
    DynamicValue: "DynamicValue",
    Factory: "Factory",
    Function: "Function",
    Instance: "Instance",
    Invalid: "Invalid",
    Provider: "Provider"
};

There was nothing preventing

//literal-types
const BindingTypeEnum: interfaces.BindingTypeEnum = {
    ConstantValue: "ConstantValue",
    Constructor: "ConstantValue",
    DynamicValue: "ConstantValue",
    Factory: "ConstantValue",
    Function: "ConstantValue",
    Instance: "ConstantValue",
    Invalid: "ConstantValue",
    Provider: "ConstantValue"
};

Now we have the concept of a ValueProvider where each of the derivations have a type that can be used for discriminated unions. ( Although it is highly unlikely, imo, we could add another one later. ).
The BindingType is a union of these types - so no change to before, it is a union of strings.
I have typed the BindingTypeEnum interface such that the class has to follow.

//interfaces
    export type ValueProviderType<TActivated> =
        ConstantValueProvider<TActivated> |
        InstanceValueProvider<TActivated> |
        DynamicValueProvider<TActivated> |
        ConstructorValueProvider<TActivated> |
        FactoryValueProvider<TActivated> |
        ProviderValueProvider<TActivated> |
        NotConfiguredValueProvider

    export type BindingType = ValueProviderType<unknown>["type"];

    export interface BindingTypeEnum {
        ConstantValue: ConstantValueProvider<unknown>["type"];
        Constructor: ConstructorValueProvider<unknown>["type"];
        DynamicValue: DynamicValueProvider<unknown>["type"];
        Factory: FactoryValueProvider<unknown>["type"];
        Instance: InstanceValueProvider<unknown>["type"];
        Provider: ProviderValueProvider<unknown>["type"];
        NotConfigured: NotConfiguredValueProvider["type"];
    }
//literal-types
const BindingTypeEnum: interfaces.BindingTypeEnum = {
    ConstantValue: "ConstantValue",
    Constructor: "Constructor",
    DynamicValue: "DynamicValue",
    Factory: "Factory",
    Instance: "Instance",
    Provider: "Provider",
    NotConfigured: "NotConfigured"
};

Please can you explain your concern.

return this._stack.pop();
}

peek(): T | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe change this function name ?
I don't think peek implies getting last element in an array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array is the implementation detail. Peek is what you would have got from pop without removing it ?

@PodaruDragos
Copy link
Contributor

left some comments, I'll review this thoroughly if we agree to make this change

@tonyhallett
Copy link
Contributor Author

@PodaruDragos
Thanks for the review.

but maybe users like .to().blabla syntax.

The to syntax has not changed. Are your referring to the properties from the Binding interface ?

    export interface Binding<TActivated> extends Clonable<Binding<TActivated>> {
        dynamicValue: DynamicValue<TActivated> | null;
        //type: BindingType;
        implementationType: Newable<TActivated> | TActivated | null;
        factory: FactoryCreator<unknown> | null;
        provider: ProviderCreator<unknown> | null;
        cache: null | TActivated | Promise<TActivated>;
    }

We could provide setters and getters.

@PodaruDragos
Copy link
Contributor

The to syntax has not changed. Are your referring to the properties from the Binding interface ?

maybe I didn't carefully read that. Is it still possible to do container.bind('bla').to('bla').inSingletonScope() ?

@tonyhallett
Copy link
Contributor Author

The to syntax has not changed. Are your referring to the properties from the Binding interface ?

maybe I didn't carefully read that. Is it still possible to do container.bind('bla').to('bla').inSingletonScope() ?

Syntax has not changed at all.
The difference is that Binding has a ValueProvider and a Scope.

@PodaruDragos
Copy link
Contributor

Syntax has not changed at all.

then I am really ok with this. thanks for clearing that out.

@tonyhallett
Copy link
Contributor Author

@PodaruDragos

I did not consider errors thrown during resolution so I think that an error handler is necessary to ensure the context stack is cleared up.
current code

    private _planAndResolve<T>(): (args: interfaces.NextArgs<T>) => interfaces.ContainerResolution<T> {
        return (args: interfaces.NextArgs<T>) => {

            // create a plan
            let context = plan(
                this._metadataReader,
                this,
                args.isMultiInject,
                args.targetType,
                args.serviceIdentifier,
                args.key,
                args.value,
                args.avoidConstraints
            );

            this._contextStack.planCompleted(context);

            // apply context interceptor 
            //**************************************************************
            context = args.contextInterceptor(context);

            // resolve plan
            const result = resolve<T>(context); 
            //**********************************************************
            this._contextStack.resolved(); 

            return result;

        };
    }

@PodaruDragos
Copy link
Contributor

@tonyhallett @notaphplover did we plan to not do this update ?
Seems like this could be a good way forward.

I do want to first maybe land this PR first to not deal with a lot of conflicts.

@tonyhallett @notaphplover what do you think ?

@tonyhallett
Copy link
Contributor Author

I'd certainly would like it to be discussed further. It adds a new scope that resolves an outstanding issue and in my opinion improves upon the current design. I may have missed something so it needs a review from a couple of people.

Re the linked draft. Apologies I have not looked at it yet. I have some open source code that I need to finish but I will have a look soon

@PodaruDragos
Copy link
Contributor

@tonyhallett hello
now that #1311 has been merged. I think it's time to go over this again.
I'll take a look again on this. I do think that merging master into this branch is gonna be a big hassle.

@tonyhallett
Copy link
Contributor Author

I do think that merging master into this branch is gonna be a big hassle

Most definitely

I have a couple of outstanding tasks that will probably keep me busy for a couple of weeks. After that I will be able to join in.

@PodaruDragos
Copy link
Contributor

hey @tonyhallett @notaphplover should be start going over this again ?

@tonyhallett
Copy link
Contributor Author

Unfortunately I am busy with other things. I would love to participate but it will not be possible for a while.

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

Successfully merging this pull request may close these issues.

2 participants