Promise (Lazy) Support + Global onActivation/onDeactivation

[parisholley]

This is a cleaner refactor which combines #1074 and #1086 to allow for users to opt-in to lazy loaded dependencies by using an asynchronous API.

For anyone wanting to play around with this, you can use the NPM release:

@parisholley/inversify-async:1.1.7

Description

New container methods for pulling objects out of the container in an asynchronous fashion while respecting container semantics and error if trying to mix synchronous/asynchronous dependencies.

Implementation

container.bind<Type>('name').toDynamicValue(async () => {
  return await myAsyncFunction();
});
container.bind<string>('resolve').toDynamicValue(() => Promise.resolve('foobar'));

const value = await container.getAsync<Type>('name'); // result of myAsyncFunction
const value = await container.get<Type>('name'); // throws an exception
const value = await container.getAsync<string>('resolve'); // foobar

Global onActivation

container.onActivation('asyncIdentifier', (context, injectable) => {
  return Promise.resolve('newObject'); // returning a promise enforces use of getAsync(), even if future binds are not async
});
container.bind('asyncIdentifier').toConstantValue('foobar');

await container.getAsync('asyncIdentifier');

container.onActivation('syncIdentifier', (context, injectable) => {
  return 'newObject'; // no promise = use legacy API
});

container.get('syncIdentifier');

Binding onActivation

container.bind('asyncIdentifier').toConstantValue('foobar').onActivation((context, injectable) => {
  return Promise.resolve('newObject'); // returning a promise enforces use of getAsync()
});

await container.getAsync('asyncIdentifier');

container.bind('asyncIdentifier').toConstantValue('foobar').onActivation((context, injectable) => {
  return 'newObject'; // no promise = use legacy API
});

container.get('syncIdentifier');

Parent/Child onActivation

container.onActivation('asyncIdentifier', (context, injectable) => {
  return Promise.resolve('newObject'); // returning a promise enforces use of getAsync()
});

const child = container.createChild();
child.bind('asyncIdentifier').toConstantValue('foobar');

await container.getAsync('asyncIdentifier');

Module onActivation

container.bind('asyncIdentifier').toConstantValue('foobar');

const TestContainerModule = new ContainerModule((bind, unbind, isBound, rebind, onActivation) => {
  onActivation('asyncIdentifier', async (context, service) => {

 }));
});

Related Issue

#418
#475
#887
#404

Motivation and Context

Use Case 1: Unit/System Testing
While you technically could create separate containers (with a parent container for common objects) for a unit test environment and system test (database, etc), it adds more code complexity and also requires each system integration be live and working in order to bootstrap the container (this matters, because for instance in the IDE, if I want to run a single test, with a single I/O dependency, I don't have to initiate every connection).

Use Case 2: Common Container Use
Let's say you have a "service" code base that is re-used between a web application, command line interface and batch processing. In the case of the CLI, you have different commands that serve different purpose, eg:

• Clone production data
• Invoke third party service

By making I/O dependencies lazy loaded (async), our CLI can wait until runtime to connect to the system that it needs for the task (otherwise, same problem as above, you are forced to connect-to, and have access-to systems that are unrelated to the command at hand).

Use Case 3: Transactions:
For testing (or perhaps a custom request based framework where child containers are created for each web request to implement "Request Scope" similar to spring), it may be useful to have async construct/destroy methods. For example, a web request where we want to run all service calls in a transaction and commit on at the end (by unbinding the container), or in tests by rolling back database changes for more performant runs.

How Has This Been Tested?

Full suite of tests have been implemented.

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.

[sublimator]

@parisholley

Hi, nice work! This looks interesting!

I am shopping around for a DI system that handles async instantiation. If I'm not mistaken, one should basically only create async providers(bind(x).toDynamicValue) for leaves in the dependency Graph right.

i.e. independent units ? Is that generally the way your apps have structured themselves ?

I'm new to inversify, but it seems what this offers over container.loadAsync(new AsyncContainerModule(...)) is, as you say, "lazy" evaluation of the providers. You don't need to create an async module for each "use case" of a component suite. Just declare one set of bindings and pull out graph subsets easily.

How has been the reception to your patch so far ? How close is this to being "finished"
Do you see it landing any time soon?

It's not apparent to me immediately what "onActivation" means.
Will have a play around with it :)

[parisholley]

@sublimator my issue with loadAsync, is it assumes you want to bootstrap all of your async dependencies in one go, OR, attempt to break up your async dependencies in various independent modules and/or containers and load as you need.

for example, you in theory could do:

root container
- config container (eg. pulling creds from persistant storage vs relying on ENV)
- i/o container (database, redis, etc)
- test container (stub a bunch of stuff)

in the above, you would loadAsync root + config and i/o on start of a web app. while that works great for that, now lets assume you want to transactions per web request, you can either use the "request scope" pattern inside Inversify or create a child container for each request and handle isolation that way.

now lets say, you have lots of I/O (in my case, I use about 10-20 different AWS products/open source systems), what realistic approach can you take with chunking up all your async dependencies? more so, what happens when all of your request endpoints may only use 2-3 at any given time and can vary a lot? you end up having a lot of "sub containers"/module for every single async scenario you need, which gets sloppy and hard to maintain (and will likely break fundamental things like pooling across the entire process). I shouldn't have to create custom containers for every web request that has unique I/O requirements.

add ANOTHER layer on top of that, where you have tests that can have varying degrees of requirements. i personally believe, i should be able to open an IDE, right click > run test, and everything needed to bootstrap THAT test, is complete. I have built my own framework (which I will open source soon), that allows me to have access to a unique container per test, while sharing a root container (for connection pooling), allowing me to run tests fast and in parallel (by utilizing transactions, virtual partioning, etc).

add ANOTHER layer :) where i have a monolith project that has both a Web layer, CLI layer (for occasional debug/one-off needs), Job layer (batch/stream processing/cron) and every Job/CLI has unique I/O requirements. I need to create containers for each of those as well?

that is a lot of complication/overthinking for just wanting an auto-built graph :)

onActivation/onDeactivation is powerful! here are many ways i use it:

• create logically partitioned tables/databases/indexes per test (table_1, table_2) for datasources that do not support transactions and run any initiations (mappings, schemas, etc)
• start transaction (onActivation), end transaction (onDeactivation) for testing (and/or requests)
• ensure databases are in correct state (eg: mongo is in primary mode so transactions can work)
• ensure cloud services are initialized before use (s3 buckets, etc) [i use the localstack project, but requirements are the same]

i can forsee people creating all types of neat middleware for bootstrapping functionality into existing containers, without having to pollute your codebase with secondary concerns.

in my ideal world, a project works like this:

git clone
docker-compose up
npm test (or run individual tests)

there is no need to worry about environment state, the async container system can boostrap for you (if you so choose). if you prefer the simple, monolith/waterfall approach to async dependencies because you only have a handful, this may seem like overkill. but when your test suite is 1K+ and your CI builds go 30mins+ on tests alone, you come to appreciate async'ness.

as to this patch, it has been a ghost town from the project owners since my first push earlier this year. i have seen some commit activity from @dcavanagh but no one has put in their two cents. i may just create a fork of project or roll it into my framework if there is no movement.

[sublimator]

Thanks the detailed explanation. Will reply tomorrow. 9pm here :)

[sublimator]

For now: “Totally makes sense to me” Fine grained (== perf etc) rather than load ALL ur async leaves ...

[sublimator]

@parisholley

it has been a ghost town from the project owners since my first push earlier this year

Hey, well I hope they take a look at it and least give some rationale for why they won't accept (if that's the case). I think your reasons for wanting async are all quite compelling and I think once people start using it, it would be "why haven't we had this sooner??". Soooo* much JavaScript code is async ...

Is it possible the maintainers are just busy? There are 17 pull requests and 152 issues competing for maintainer attention.

[sublimator]

This isn't that massive a change huh ?

[parisholley]

nope :) i hacked up half the project the first time around trying to get this to work, and then realized it would be as simple as promise detection.

[sublimator]

Ponder points:

1. 3rd party Promise libs. Do they generally just inherit from Promise ? I never use them, but I guess some people (still?) do.
2. What happens when you have "concurrent" async requests going on at once for a given container (Say a parent container, and per http request child containers accessing the parent container for 'shared' deps). Is that supported ? Does this do the right thing in terms of honoring the scopes etc ?

[parisholley]

1. i'd assume if anyone is using a 3rd party lib, they are probably doing so as a polyfill, which would override window.Promise, and this should still be true.

2. from what I understand of my latest implementation, if there is a race condition with async, it would also exist with non-async (eg: dynamic values, etc). the same semantics of how things are looked up in parent containers and/or cached on bindings ring true with promises.

[sublimator]

I see, thanks :) So the promises themselves are cached if I am understanding correctly?

[parisholley]

Yep

…

Sent from my iPhone

On Sep 7, 2019, at 3:48 AM, Nicholas Dudfield ***@***.***> wrote: @sublimator commented on this pull request. In src/resolution/instantiation.ts: > const constructorInjectionsRequests = childRequests.filter((childRequest: interfaces.Request) => (childRequest.target !== null && childRequest.target.type === TargetTypeEnum.ConstructorArgument)); const constructorInjections = constructorInjectionsRequests.map(resolveRequest); + if (constructorInjections.some((ci) => ci instanceof Promise)) { I see, thanks :) So the promises themselves are cached if I am understanding correctly? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[tonyhallett]

I don't think that you should be using instanceof Promise. How do I tell if an object is a promise.

promisfied.ts

import {Promise} from "bluebird"
function toPromisfy(callback:(error:any,success?:string)=>void){
    callback(undefined,"Resolved");
}
export const promisfied=Promise.promisify(toPromisfy);

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

describe("bluebird Promisfy",()=>{
    it("should not work with instanceof",()=>{
        const isThisAPromise=promisfied();
        expect(isThisAPromise).not.toBeInstanceOf(Promise);
        expect(isThisAPromise.then).toBeDefined();
    })
})

[parisholley]

@tonyhallett while that is true, i don't see why anyone would explicitly switch between native/non-native and choose not to use polyfills. i suspect babel and most manual implementations do something like this:

import {Promise} from "bluebird"
global.Promise = Promise;

[parisholley]

@tonyhallett just got bit by this bug on lambda... looks like thenable/A+ check it is :)

aws/aws-xray-sdk-node#27

[notaphplover]

@notaphplover Are you ok if I finish this all off ? Wiki and tests ? I will leave if(context.plan) in case dev has messed with context in middleware and cover instead.

Sure, I should have done it yesterday but I was kind of tired, sry

[tonyhallett]

Sure, I should have done it yesterday but I was kind of tired, sry

Not at all ! I will tie up these loose ends and let's get this feature in !

I will then have a pull request that removes scope management from the resolver. This facilitates a root request scope #1143 as well as custom scopes. If you could review that if you have time that would be great.

[tonyhallett]

@notaphplover when you have time let's get this in !

[notaphplover]

Excellent job! Thank you so much @tonyhallett , I think we should avoid to change one interface but everything else is great! Would you agree if I undo the change ar .createChild() ?

[tonyhallett]

@notaphplover

I think we should avoid to change one interface but everything else is great! Would you agree if I undo the change ar .createChild() ?

I think that it is highly unlikely to cause any issues but yes let's leave it for another time.

Please go ahead and revert the change and, dare I say it, merge it in !

[notaphplover]

@notaphplover

I think we should avoid to change one interface but everything else is great! Would you agree if I undo the change ar .createChild() ?

I think that it is highly unlikely to cause any issues but yes let's leave it for another time.

Please go ahead and revert the change and, dare I say it, merge it in !

I'm writing @dcavanagh now, he is the one who should merge this :)

[tonyhallett]

@notaphplover
We should really sort this out.

[tonyhallett]

There is a problem here due to the typing

public toConstructor<T2>(constructor: interfaces.Newable<T2>): interfaces.BindingWhenOnSyntax<T> {
        this._binding.implementationType = constructor as any;
       ...
    }

[notaphplover]

I think we should add breaking changes to solve it. Do you have anything in mind?

We have plans to change typings, but I think we should accept they are not perfect :(

[tonyhallett]

Do you have anything in mind?

It will need some thought for sure !

I think we should add breaking changes to solve it

That will probably be necessary.

It's only a problem as resolveRequest is typed to return undefined | T | Promise<T> | (T | Promise<T>
All other returns are typed as any !

Perhaps better to untype the return for now.

Problems still remain but they were there before this feature.

I will happily look at the typings at a later date.

What I would like to do is to remove all of this code !

if (binding.type === BindingTypeEnum.ConstantValue) {
            result = binding.cache;
            binding.activated = true;
        } else if (binding.type === BindingTypeEnum.Function) {
            result = binding.cache;
            binding.activated = true;
        } else if (binding.type === BindingTypeEnum.Constructor) {
            result = binding.implementationType as unknown as T;
        } else if (binding.type === BindingTypeEnum.DynamicValue && binding.dynamicValue !== null) {
            result = invokeFactory(
                "toDynamicValue",
                binding.serviceIdentifier,
                () => (binding.dynamicValue as (context: interfaces.Context) => any)(request.parentContext)
            );
        } else if (binding.type === BindingTypeEnum.Factory && binding.factory !== null) {
            result = invokeFactory(
                "toFactory",
                binding.serviceIdentifier,
                () => (binding.factory as interfaces.FactoryCreator<any>)(request.parentContext)
            );
        } else if (binding.type === BindingTypeEnum.Provider && binding.provider !== null) {
            result = invokeFactory(
                "toProvider",
                binding.serviceIdentifier,
                () => (binding.provider as interfaces.Provider<any>)(request.parentContext)
            );
        } else if (binding.type === BindingTypeEnum.Instance && binding.implementationType !== null) {
            result = resolveInstance(
                binding,
                binding.implementationType,
                childRequests,
                _resolveRequest(requestScope)
            );
        } else {
            // The user probably created a binding but didn't finish it
            // e.g. container.bind<T>("Something"); missing BindingToSyntax
            const serviceIdentifier = getServiceIdentifierAsString(request.serviceIdentifier);
            throw new Error(`${ERROR_MSGS.INVALID_BINDING_TYPE} ${serviceIdentifier}`);
        }

and instead binding.provideValue(context) and have binding as abstract with a derivation for each of the BindingToSyntax distinguishable methods.
( requestScope not necessary with the scope feature I want to add when this goes through )

Perhaps this would help.

[notaphplover]

Well T is supposed to be the result's type, I understand your feeling, I really want to improve that code's block, but (T | Promise | (T|Promise)[]) is a valid returning type. A constructor binding is suposed to be called with a T extends Newable<unknown>, considering that (T | Promise | (T|Promise)[]) is a valid return type.

I'm looking forward to improve that code, but I believe achieving changes through small PR is the best way to go. It's easier to revier a small PR, it's easier to solve conflicts so it helps this project to scale. So I prefer to solve this in another PR

[tonyhallett]

We can discuss the return type at a later date. I agree another PR for type changes.

[tonyhallett]

Also please see #1319

[sublimator]

@tonyhallett @notaphplover

Great, I see this was merged! :)
Any idea when will be released to npm ?

[infloop]

Hi! @dcavanagh! Super-great DI library! We are looking forward to use these async dependencies! Are there any chance that it will be published to npm soon?

[sublimator]

@tonyhallett @notaphplover @dcavanagh

Cheeky bump :)

[jakehamtexas]

I hope this gets released soon - the documentation on master claims this functionality exists per this four month old commit: e7c7943

Very happy to be using the existing functionality in the meantime! I know that the life of an OSS dev is fraught with grind and thanklessness, I appreciate everybody's effort so much for this 🙏

[infloop]

Up!