Type instantiation is excessively deep and possibly infinite.ts(2589)

[KennethKo]

After the 50th or so .provideFoo( call on an injector, ts starts to complain about the deeply nested TChildContext<...>s.

https://github.com/microsoft/TypeScript/issues/34933

Is there no way to bulk provide classes to the injector, or perhaps a better provide signature besides nesting child contexts?

[nicojs]

Hi @KennethKo 👋 and welcome.

How would you like the signature of "bulk provide" to work? Do you have suggestions?

[KennethKo]

Wrt the parameter signature, anything would be fine: .provideClasses(token1, class1, token2, class2...) .provideClasses([[token1, class1], [token2, class2], ...]) .provideClasses({ [token1]: class1, [token2]]: class2, ...})

Though imo, the token keyed class object collection would be ideal.

As for the signature of the returned ChildContext, I'm really not sure what Typescript would allow. Ideally, it would let you get away with

provideClasses<TokenPairs extends { [string] : Class>(TokenPairs tps) : ChildContext<TContext, TokenPairs>

...and preserve the static typing of the passed pairs. Even so, I’m not sure how you would have to modify your static validation to allow for multiple shapes of context or looking up tokens from a collection.

Also not sure if it scales past 50 to solve the actual problem, but intuitively, it should, and it should const time lookup instead of linear anyway.

On Tue, Aug 11, 2020 at 6:29 AM Nico Jansen notifications@github.com wrote:

Hi @KennethKo https://github.com/KennethKo 👋 and welcome.

How would you like the signature of "bulk provide" to work? Do you have suggestions?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nicojs/typed-inject/issues/22#issuecomment-671947178, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHRLM3XGLVVS5ZYUSWHEIHLSAFBTDANCNFSM4NHNIASQ .

[nicojs]

Though imo, the token keyed class object collection would be ideal.

This is what I was thinking of as well. How about something like this?

class Foo {};
function bar() { return 'bar' };
value baz;
injector.provide({
  foo: { class: Foo },
  bar: { factory: bar, scope: Scope.Transient },
  baz: { value: baz }
});

We could allow for additional options, like scope to allow to override the default scope.

This will work in practice, however, there will be a limitation as well. For example, this will not work:

class Foo {
  static inject = tokens('bar');
  constructor(public bar: string) {} ;
};
function bar() { return 'bar' };
injector.provide({
  foo: { class: Foo },
  bar: { factory: bar },
});

In this example, Foo needs bar. This could be resolved at runtime but would be impossible to do at compile time.

What do you you think @KennethKo ?

[KennethKo]

It's all very uncomfortable, this signature design. I can only speak to my own experience w/ (and my own usage of) your platform, but I can give you a gut check on my intuition.

foo: { class: Foo }, -is nice and robust and flexible, but it's also verbose. I suspect this would discourage its usage, and the recurrence of the "{ class: " reserved string would make eyes glaze over.

The ideal would be to duck type the provided Rs in place... > foo: fooClass, bar: barFactory but then it would probably be impossible for the ts compiler to disambiguate types at compile time there. In which case, I would bias more towards a set of direct "provideClasses", "provideFactories", and "provideValues" signatures, as I found that our projects would rarely mix factories and classes. However, this encourages a different sort of antipattern, tied to your note that dependencies cant be resolved within the same bulk-provide at compile time. On one hand, given that you can't resolve dependencies within a bulk provide, it decreases the need to mix providing classes and factories and values within the same call. On the other hand, encouraging people to structure their dependencies like this: >.provideValues({ ...configValuesForAllClasses}) >.provideClasses({ ...baseDependencyClasses}) >.provideClasses({ ...level2Classes}) is an antipattern. It moves dependencies farther away from the things they depend on, and it makes it more difficult to adapt the configuration when a class's dependency tree changes (moving from level3Classes to level1 or whatever). So I threw bulkProvide out there as a stopgap solution, but on reflection, it probably encourages bad practice long term. As a base case, I found that the most common pattern we fell into was, for each class, to provide a value to config it and to then provide the class itself. To this end, individual calls to inject were, in fact, ideal. The only real problem here was the return signature, with ChildContexts nested 50 deep. It was slow, and it made the compiler unhappy. The real ideal here would be for the single-provide methods to adapt the context they return more cleverly. Instead of returning a context that nests the current context, perhaps they should all attempt to simply mutate the TokenPairs bag in the context object: > provideClass<NewToken extends string, NewClass extends Class, TokenPairs extends { [string] : Class}>(token: NewToken, foo: NewClass) : ChildContext<OldTContextForEdgecases, TokenPairs & { NewToken: NewClass }>

How you ultimately structure TokenPairs will probably come down to implementation details. I don't remember exactly, but I think you might need to separate the collection out into { values: TokenValuePairs, classes: TokenClassPairs, ...} or some such silliness.

(I don't remember exactly if TokenPairs needs to extend { [string] : Class } to start, or if that would sabotage it. I haven't been in ts mode for some time. But I'm fairly confident that a clean solution exists somewhere.)

On Wed, Aug 12, 2020 at 11:21 AM Nico Jansen notifications@github.com wrote:

Though imo, the token keyed class object collection would be ideal.

This is what I was thinking of as well. How about something like this?

class Foo {};function bar() { return 'bar' };value baz;injector.provide({ foo: { class: Foo }, bar: { factory: bar, scope: Scope.Transient }, baz: { value: baz }});

We could allow for additional options, like scope to allow to override the default scope.

This will work in practice, however, there will be a limitation as well. For example, this will not work:

class Foo { static inject = tokens('bar'); constructor(public bar: string) {} ;};function bar() { return 'bar' };injector.provide({ foo: { class: Foo }, bar: { factory: bar },});

In this example, Foo needs bar. This could be resolved at runtime but would be impossible to do at compile time.

What do you you think @KennethKo https://github.com/KennethKo ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nicojs/typed-inject/issues/22#issuecomment-673034820, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHRLM3UGNCVDVGTXJK7D24DSALMTDANCNFSM4NHNIASQ .

[nicojs]

I'm closing this PR as I don't think this is needed. Having multiple ways of doing things isn't part of the API. I also think the error messages TypeScript provides are getting better. Feel free to open new issues if you think of other API improvements or ways to improve the error messages.

[nsb]

Love this library. Thanks a lot! But I also just hit this issue. I guess this needs to be addressed or highlighted in the docs. I don't know how to overcome this without ripping out typed-inject.