Binding typing discussion

[tonyhallett]

@notaphplover

With respect to #1132 (comment)

This is what I think the correct typing should be after #1132. Omitting unnecessary properties.

export interface Binding<T> extends Clonable<Binding<T>> {
        serviceIdentifier: ServiceIdentifier<T>;
        dynamicValue: DynamicValue<T> | null;
        implementationType: T | Newable<T> | null; //**********************
        factory: FactoryCreator<any> | null;
        provider: FactoryCreator<any> | null;
        onActivation: BindingActivation<T> | null;
        onDeactivation: BindingDeactivation<T> | null;
        cache: T | Promise<T> | null; // ( mentioned in review )
    }

The Binding class is currently incorrect with factory and provider.

It would be great to remove T2 from toConstructor
public toConstructor<T2>(constructor: interfaces.Newable<T2>): interfaces.BindingWhenOnSyntax<T> {
and change to either
constructor:T
or constructor:Newable<any>

Both statements result in an incorrect onActivation

container.bind<interfaces.Newable<Category>>("Newable<Category>").toConstructor(Not);
container.bind<interfaces.Newable<Category>>("Newable<Category>").toConstructor<Not>(Not)

By correcting there is no need to cast

public toConstructor<T2>(constructor: interfaces.Newable<T2>): interfaces.BindingWhenOnSyntax<T> {
        this._binding.type = BindingTypeEnum.Constructor;
        this._binding.implementationType = constructor as any;
        this._binding.scope = BindingScopeEnum.Singleton;
        return new BindingWhenOnSyntax<T>(this._binding);
    }

The implementationType needs to change from
implementationType: Newable<T> | null;
to implementationType: T | Newable<T> | null;
because of
container.bind<interfaces.Newable<Category>>
and resolver ( note that we are already changing the type T )

InversifyJS/src/resolution/resolver.ts

Line 84 in d06d0b0

result = binding.implementationType as unknown as T;

InversifyJS/src/resolution/resolver.ts

Line 117 in d06d0b0

if (isPromise(result)) {

we are activating T

        if (isPromise(result)) {
            result = result.then((resolved) => _onActivation(request, binding, resolved));
        } else {
            result = _onActivation(request, binding, result);
        **}**

[notaphplover]

Hi @tonyhallett , I think it's a good approach, should we create an v6 branch to address all the PR related to breaking changes? This way we could avoid releasing too many major versions

[tonyhallett]

The Promise (Lazy) Support + Global onActivation/onDeactivation feature is a breaking change ( well it will be when the cache type is corrected by Refactor resolve request ) and we should release v6 now.

[notaphplover]

@tonyhallett If we are about to break, I would wait before releasing v6. We really want to improve typings, some additional BC are about to be done and I think we should try not to launch a v7 too quickly

[tonyhallett]

By all means wait for release, but The Promise (Lazy) Support + Global onActivation/onDeactivation feature is a breaking change ( with the fix we are putting through ) so shouldn't we be updating package.json ?

[notaphplover]

By all means wait for release, but The Promise (Lazy) Support + Global onActivation/onDeactivation feature is a breaking change ( with the fix we are putting through ) so shouldn't we be updating package.json ?

Ahh, well Im not sure. What are we breaking? We add async support but I cant think in a piece of code which would stop working as long as the syncronous behavior remains unchanged

[tonyhallett]

What are we breaking ?

The interface Binding has changed.
The binding interface is available to middleware through the context through the context interceptor and also to factory creators.
Context -> Request -> Binding<any>

Beforehand it would be possible ( and I am not saying this is what you would do ) to cast the Binding and do below

const binding:interfaces.Binding<Blade> = null as any; // cast the binding
const constructed = new binding.implementationType!()

Now you get the error message
This expression is not constructable.
Not all constituents of type 'Blade | Newable' are constructable.
Type 'Blade' has no construct signatures.

A contrived example with cache

class Cached {
        doSomething():void {}
      }
      const middleware:interfaces.Middleware = (next) => {
        return (args:interfaces.NextArgs) => {
            args.contextInterceptor = context => {
                const binding:interfaces.Binding<Cached> = context.currentRequest.bindings[0];
                binding.cache!.doSomething();
                return context;
            }
            return next(args);
        }
      }

Now you get the error message
Property 'doSomething' does not exist on type 'Cached | Promise'.
Property 'doSomething' does not exist on type 'Promise'.

I don't know what people are doing with the Context but with the change it is possible to break existing code,

[tonyhallett]

should we create an v6 branch to address all the PR related to breaking changes? This way we could avoid releasing too many major versions

Yes - https://github.com/inversify/InversifyJS/tree/v6

and https://github.com/inversify/InversifyJS/tree/v6-binding-abstractions as per #1327 (comment)