-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(context): add @inject.tag to allow injection by a tag #857
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
The PR will be enhanced with ResolutionSession once #852 is landed.
I think it may be better to land this right now and apply resolution session tracking in a follow-up pull request. Whatever works for you best.
| await ctx.get('store'); | ||
| throw new Error('An error should have been thrown'); | ||
| } catch (e) { | ||
| expect(e.message).to.eql('Bad'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use rejectedWith as we discussed recently in another pull request.
| values[i] = val; | ||
| } | ||
| } | ||
| if (asyncResolvers.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably out of scope of this pull request: I feel we are repeating the asyncResolvers pattern in too many places. It would be good to extract a shared helper function.
A mock-up to illustrate my point:
function resolveByTag(ctx: Context, injection: Injection) {
const tag: string | RegExp = injection.metadata!.tag;
const bindings = ctx.findByTag(tag);
return mapValuesOrPromises(bindings, b => b.getValue(ctx));
}
function mapValuesOrPromises<T, O>(
list: ValueOrPromise<T>,
fn: (item: T) => ValueOrPromise<O>,
): ValueOrPromise<O[]> {
const values: O[] = new Array(list.length);
const asyncResolvers: PromiseLike<O>[] = [];
const valSetter = (i: number) => (val: BoundValue) => (values[i] = val);
// tslint:disable-next-line:prefer-for-of
for (let i = 0; i < bindings.length; i++) {
const val = fn(list[i]);
if (isPromise(val)) {
asyncResolvers.push(val.then(valSetter(i)));
} else {
values[i] = val;
}
}
return asyncResolvers.length ?
Promise.all(asyncResolvers).then(_ => values):
values;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. See #871
1f258d2 to
49325fd
Compare
49325fd to
a02345f
Compare
Add
@inject.tagdecorator to allow injection of an array of bound values by a tag.This DI can be used with the extension point/extension pattern so that the extension point can receive injection all extensions.
This is a spin-off from #671. The PR will be enhanced with ResolutionSession once #852 is landed.
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated