Track dependency resolution #852
Conversation
|
The CI fails due to #853 |
There was a problem hiding this comment.
Thank you for this pull request, it's much easier for me to review this smaller set of changes 👍
The changes in packages/context/src/inject.ts look too cryptic to me, I'll trust you here.
I reviewed the changes up to packages/context/test/unit/resolver.test.ts, I try to finish it ASAP.
I have one concern though: in session, you are storing injections and bindings as two distinctive arrays (stacks). Isn't it possible though to have a path that contains both injections and bindings? E.g. "resolve binding X via ctx.get" -> "resolve binding Y via custom injection" -> "resolve binding Z via "ctx.get". Will we print a correct path in such case? If this is already handled then please point me to the unit test(s) covering this scenario.
packages/context/src/binding.ts
Outdated
| resolutionSession, | ||
| ); | ||
| if (isPromise(result)) { | ||
| if (result instanceof Promise) { |
There was a problem hiding this comment.
I believe this is redundant and should be removed. We have already checked that result is a promise, plus instanceof Promise does not recognize user-land promise libraries like Bluebird.
There was a problem hiding this comment.
Unfortunately, isPromise() only ensures the result object is PromiseLike, which has then(), but not catch().
packages/context/src/binding.ts
Outdated
| @@ -237,7 +253,25 @@ export class Binding { | |||
| } | |||
| } | |||
| if (this._getValue) { | |||
| const result = this._getValue(ctx); | |||
| const resolutionSession = ResolutionSession.enterBinding(this, session); | |||
| let result: ValueOrPromise<BoundValue> = this._getValue( | |||
There was a problem hiding this comment.
Is it possible for _getValue to throw? Even if not today, can it happen in the future, e.g. because of a TypeError: undefined is not a function or similar?
I think we should wrap this code in a try/catch block and exit the session in the catch block.
const resolutionSession = ResolutionSession.enterBinding(this, session);
try {
let result: ValueOrPromise<BoundValue> = this._getValue(/*...*/);
} catch (err) {
resolutionSession.exit();
throw err;
}
if (isPromise(result)) {
// etc.I have an idea how to design ResolutionSession in a way that would not require us to call exit at all: instead of using a mutable state in ResolutionSession to keep the list of bindings visited, we should create a new session object for each binding visited instead, with a pointer to the previous (parent) session. That way session.binding becomes a regular property stored in the session, and session.bindings() will be computed by traversing the linked list of all parent sessions. I think this will also remove the need for session.clone(), since each session instance will be immutable. (A better name than "session" may be needed too.)
However, in order to speed up the review process, I am ok to leave that proposed improvement out of scope of this pull request. I am actually happy to implement it myself once this patch gets landed.
There was a problem hiding this comment.
At the moment, we abort the resolution session if any sync/async error happens. Try-Catch is not absolutely needed. I don't mind making it consistent.
packages/context/src/resolver.ts
Outdated
| * Object to keep states for a session to resolve bindings and their | ||
| * dependencies within a context | ||
| */ | ||
| export class ResolutionSession { |
There was a problem hiding this comment.
Please move ResolutionSession to its own file, src/resolver.ts is already too long. The fact that you need two debug loggers (debug and debugSession) is IMO a good indicator supporting my claim.
packages/context/src/resolver.ts
Outdated
| * A stack of bindings for the current resolution session. It's used to track | ||
| * the path of dependency resolution and detect circular dependencies. | ||
| */ | ||
| readonly bindings: Binding[] = []; |
There was a problem hiding this comment.
Having two property names that differ only in the last letter (binding vs. bindings) makes code difficult to read, at least to me, because the distinction is so easy to overlook.
I am proposing more descriptive names. While I prefer both properties to be renamed, I don't mind too much if you decide to rename only one of them.
- rename
bindingstopath,stack,parentBindings, or similar - rename
bindingtoo, for example tocurrentBinding
The same comment applies to injection vs. injections.
packages/context/src/resolver.ts
Outdated
| if (injection.resolve) { | ||
| // A custom resolve function is provided | ||
| return injection.resolve(ctx, injection); | ||
| resolved = injection.resolve(ctx, injection, session); |
There was a problem hiding this comment.
What if injection.resolve throws an error (synchronously)? I think this line should be wrapped with a try/catch block to exit the session on errors.
packages/context/src/resolver.ts
Outdated
| .catch(e => { | ||
| session!.exitInjection(); | ||
| throw e; | ||
| }); |
There was a problem hiding this comment.
Please add a comment explaining that this is a workaround for missing Promise.prototype.finally? Note that support for this has been landed to TypeScript recently (see microsoft/TypeScript#20511). Unfortunately it hasn't been released yet.
I would consider extracting a helper function to keep the code in resolve() shorter and easier to understand.
return andFinally(resolved, () => session!.exitInjection());
packages/context/src/resolver.ts
Outdated
| session!.exitInjection(); | ||
| return r; | ||
| }) | ||
| .catch(e => { |
There was a problem hiding this comment.
I think you should be using a two-callback version of then here. Right now, if exitInjection throws an error, then this catch handler is invoked for that error, and exitInjection is called again.
resolved.then(
r => { /*... */ },
err => { /*... */});| ); | ||
| }); | ||
|
|
||
| it('will not report circular dependencies if two bindings', () => { |
There was a problem hiding this comment.
if two bindings do what? I think the test name is missing something?
raymondfeng
force-pushed
the
track-dependency-resolution
branch
4 times, most recently
from
d4d22e4 to
6035489
Compare
I had two separate stacks to track bindings and injections. As you pointed out, there is a possibility that some of the binding resolutions are not from injections. I didn't intend to correlate the two stacks. |
| * Push the injection onto the session | ||
| * @param injection Injection | ||
| */ | ||
| enterInjection(injection: Injection) { |
There was a problem hiding this comment.
can we rename this method to something like pushToInjectionStack or something like that? It could be a bit confusing with https://github.com/strongloop/loopback-next/pull/852/files#diff-13e19eacf303fe8ec33cfea0245346d3R56
| return session; | ||
| } | ||
|
|
||
| static describeInjection(injection?: Injection) { |
There was a problem hiding this comment.
should we add a return type here?
There was a problem hiding this comment.
TypeScript will infer the return type. But for clarity of APIs, we can make it explicit.
|
I didn't mean to request changes! But I'm still doing another round of reviews soon. |
raymondfeng
force-pushed
the
track-dependency-resolution
branch
from
601ecbf to
09b362c
Compare
I am concerned about UX implications of such solution, but I am ok to improve this later, out of scope of this initial pull request. Incremental baby steps FTW ✌️ |
There was a problem hiding this comment.
LGTM 👍
I am little bit confused about the distinction between "binding" and "injection" paths, the two tests look almost identical to me:
expect(bindingPath).to.eql('x --> y --> z');
// vs.
expect(injectionPath).to.eql(
'XClass.constructor[0] --> YClass.constructor[0] --> ZClass.prototype.myProp',
);The thing I don't understand is why are there two different formats?
These test cases are executing the following sequences of binding/injection:
- XClass depends on YClass via bindings, YClass depends on 'Zclass' via bindings, ZClass depends on
pvia binding getter - XClass depends on YClass via bindings, YClass depends on 'Zclass' via bindings, ZClass depends on
pvia injection
I feel we are missing a test for "via injection" to "via binding" transition - I am thinking about something like this:
class ZClass {
constructor(
@inject('p', {}, (c, injection, session) => c.get('someComputedKey, session) public p: string) {}
}
|
@raymondfeng please get @kjdelisle approval before landing this patch too. |
There was a problem hiding this comment.
Looks great! Just a couple points:
- Lots of manual concatenation and use of debug's string formatting; we should replace it with the in-line formatting for clarity and consistency
- A few tests are using try-catch with expect instead of expect.throw; if the try doesn't fail, the test will pass.
| @@ -302,6 +327,10 @@ export class Binding { | |||
| * ``` | |||
| */ | |||
| toDynamicValue(factoryFn: () => BoundValue | Promise<BoundValue>): this { | |||
| /* istanbul ignore if */ | |||
| if (debug.enabled) { | |||
| debug('Bind %s to dynamic value:', this.key, factoryFn); | |||
There was a problem hiding this comment.
Use the infix string formatting instead.
debug(`Bind ${this.key} to dynamic value: ${factoryFn}`);
There was a problem hiding this comment.
It's not always the best to use infix string formatting for a few reasons:
- Infix string incurs calculation immediately, for example,
debug('%s', complexObjectWithExpensiveToString); - Infix string only uses
toString, which is not ideal for objects
| @@ -324,11 +353,16 @@ export class Binding { | |||
| * @param provider The value provider to use. | |||
| */ | |||
| public toProvider<T>(providerClass: Constructor<Provider<T>>): this { | |||
| /* istanbul ignore if */ | |||
| if (debug.enabled) { | |||
| debug('Bind %s to provider %s', this.key, providerClass.name); | |||
| try { | ||
| await instantiateClass(TestClass, ctx); | ||
| } catch (e) { | ||
| expect(e.message).to.eql('foo: error'); |
There was a problem hiding this comment.
Change this try-catch to be an expect.throw instead.
There was a problem hiding this comment.
Unfortunately, expect.throw cannot allow await.
There was a problem hiding this comment.
Have you considered using Assertion#rejectedWith() instead? I think something like this should work:
await expect(instantiateClass(TestClass, ctx))
.to.be.rejectedWith('foo: error');| try { | ||
| await instantiateClass(TestClass, ctx); | ||
| } catch (e) { | ||
| expect(e.message).to.eql('bar: error'); |
| } else { | ||
| name = 'property ' + name + '.' + member.toString(); |
There was a problem hiding this comment.
Change all of these to the inline format as well.
raymondfeng
force-pushed
the
track-dependency-resolution
branch
from
b585686 to
2ca311b
Compare
|
@bajtos I went ahead to address your last concern. Now we use one stack to track both bindings and injections as resolution elements. ResolutionSession.getResolutionPath() will give us the full path, such as: Please note |
raymondfeng
force-pushed
the
track-dependency-resolution
branch
4 times, most recently
from
b6a854c to
502f1f6
Compare
packages/context/src/inject.ts
Outdated
| session?: ResolutionSession, | ||
| ) { | ||
| // We need to clone the session for the getter as it will be resolved later | ||
| session = ResolutionSession.folk(session); |
There was a problem hiding this comment.
I find the name folk confusing, did you perhaps mean fork?
I was not able to find any dictionary entry for folk as a verb, it's typically used as a noun or adjective, see e.g. https://www.thefreedictionary.com/folk and https://dictionary.cambridge.org/dictionary/english/folk
| // Catch the rejected promise to avoid | ||
| // (node:60994) UnhandledPromiseRejectionWarning: Unhandled promise | ||
| // rejection (rejection id: 1): error | ||
| p.catch(e => {}); |
There was a problem hiding this comment.
Good catch, thank you for fixing this after me! 👍
There was a problem hiding this comment.
Thank you for addressing my concerns. I have few more comments, see the one above about folk and the one below about ResolutionElement.
I think you should be also using Assertion#rejectedWith() instead of try-await-catch, something along the following lines:
await expect(instantiateClass(TestClass, ctx))
.to.be.rejectedWith('foo: error');| get currentInjection(): Injection | undefined { | ||
| for (let i = this.stack.length - 1; i >= 0; i--) { | ||
| if (this.stack[i].type === 'injection') | ||
| return <Injection>this.stack[i].value; |
There was a problem hiding this comment.
The necessity for explicit type cast shows a problem in the design of ResolutionElement, because it allows invalid combinations like {type: 'binding', value: someInjection}.
Have you considered using a Tagged Union Types to more accurately describe the allowed values and get better type checking from TypeScript?
export interface BindingResolution {
type: 'binding';
value: Binding;
};
export interface InjectionResolution {
type: 'injection';
value: Injection;
};
export type ResolutionElement = BindingResolution | InjectionResolution;See https://blog.mariusschulz.com/2016/11/03/typescript-2-0-tagged-union-types and https://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions
raymondfeng
force-pushed
the
track-dependency-resolution
branch
from
502f1f6 to
36d63af
Compare
The current session will be cloned to create an isolated scope for the getter function to resume
- Refactor ResolutionSession into its own file - Ensure session.exit is called when an error is thrown - Fix test names
raymondfeng
force-pushed
the
track-dependency-resolution
branch
from
36d63af to
156a4b6
Compare
|
@bajtos Comments addressed. PTAL. |
| return this.bindings.map(b => b.key).join(' --> '); | ||
| return this.stack | ||
| .filter(i => i.type === 'binding') | ||
| .map(b => (<Binding>b.value).key) |
There was a problem hiding this comment.
Is this type cast still required?
| .filter(i => i.type === 'injection') | ||
| .map( | ||
| i => | ||
| ResolutionSession.describeInjection(<Injection>i.value)!.targetName, |
There was a problem hiding this comment.
Is this type cast still required?
There was a problem hiding this comment.
Yes. TS is not smart enough to understand the array filtering criteria.
Add ability to track dependency resolution and detect circular dependencies. This is a spin-off from #671.
See #535
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated