Binding typing discussion

[tonyhallett]

@notaphplover

With respect to https://github.com/inversify/InversifyJS/pull/1132#discussion_r623367620

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 ) https://github.com/inversify/InversifyJS/blob/d06d0b05d8aeca4973b2505a9ecc1ca7a13402c8/src/resolution/resolver.ts#L84 https://github.com/inversify/InversifyJS/blob/d06d0b05d8aeca4973b2505a9ecc1ca7a13402c8/src/resolution/resolver.ts#L117 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<Blade>' 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<Cached>'. Property 'doSomething' does not exist on type 'Promise<Cached>'.

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 https://github.com/inversify/InversifyJS/issues/1327#issuecomment-830864841