-
Notifications
You must be signed in to change notification settings - Fork 22
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
Type instantiation is excessively deep and possibly infinite.ts(2589) #22
Comments
Hi @KennethKo 👋 and welcome. How would you like the signature of "bulk provide" to work? Do you have suggestions? |
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 ***@***.***> 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
<#22 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHRLM3XGLVVS5ZYUSWHEIHLSAFBTDANCNFSM4NHNIASQ>
.
|
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 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, What do you you think @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 ***@***.***> 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
<#22 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHRLM3UGNCVDVGTXJK7D24DSALMTDANCNFSM4NHNIASQ>
.
|
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. |
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 |
After the 50th or so
.provideFoo(
call on an injector, ts starts to complain about the deeply nestedTChildContext<...>
s.microsoft/TypeScript#34933
Is there no way to bulk provide classes to the injector, or perhaps a better
provide
signature besides nesting child contexts?The text was updated successfully, but these errors were encountered: