Remove injector from parent on dispose

[tynor]

Resolves #37

This might not be acceptable, but it felt like the simplest solution is to use a map for tracking child injectors.

While I haven't run performance regression testing, I believe this change does slow down pretty much every operation involving children:

• provide* went from an array push to a map set, which I believe is slower
• provide* also now constructs a symbol and an arrow function (also an array temporarily for a return value, though this can be inlined)
• Dispose now must iterate over the map using for...of rather than the built-in map
• Dispose now includes a map delete (but clearing the child injectors is now unnecessary)

Initially I wanted to keep the array for storing child injectors, but tracking indices would be a nightmare.

What do you think?

[tynor]

@nicojs Just another ping on this to see if you have any thoughts.

[nicojs]

Thanks for the ping. Just came back from a long vacation and didn't get through all the notifications yet ✉.

A couple of questions about the implementation.

• Why do we need a value removeFromParent to be injected in each Injector? I think we could also add a protected removeChild(child) to the AbstractInjector class and call that (this.parent.removeChild(this)).
• Why do we use a Map<symbol, Injector> to keep children? I think we could simply keep using an array and use this.children.splice(this.children.indexOf(child), 1) (pseudo code).

[nicojs]

Also: should we maybe add an integration test that runs into the memory leak in the current version, but is fixed in the new version?

[tynor]

Thanks for the ping. Just came back from a long vacation and didn't get through all the notifications yet ✉.

Ah, I hope you had a relaxing vacation!

Why do we need a value removeFromParent to be injected in each Injector? I think we could also add a protected removeChild(child) to the AbstractInjector class and call that (this.parent.removeChild(this)).

I'd be fine with that implementation. I think I initially tried that, but I remember the method needed to be public, and I didn't want to pollute the API. Though maybe there is something I missed that would allow it to be protected.

Why do we use a Map<symbol, Injector> to keep children? I think we could simply keep using an array and use this.children.splice(this.children.indexOf(child), 1) (pseudo code).

As the system scales up, it would be nice is disposing an injector was O(n) with respect to its transitive children, not with respect to the number of children its parent has. The former is easier to shield from the effect of the environment. (The way I use dependency injection, the number of children in the base injector would scale up with the number of concurrent requests. Though maybe splicing the 0th element out of an e.g. 50,000 element array is fast enough, I haven't benchmarked it specifically.)

The reason for the Map<symbol, Injector> rather than i.e. Set<Injector> is that with the callback (vs adding a removeChild method to the parent), there is a circular dependency between the child and the callback. So that can be cleaned up if the removeChild method is added.

Also: should we maybe add an integration test that runs into the memory leak in the current version, but is fixed in the new version?

I did add a rough test to the unit tests (in test/unit/Injector.spec.ts) that fails before and passes after the patch. Though maybe something a bit more robust could be added at a higher level.

[nicojs]

I think I initially tried that, but I remember the method needed to be public

I've noticed that as well. It's a strange behavior (bug?) in typescript. We can either do another cast, or use internal and --stripInternal.

I think using a removeChild with a Set would be easier to reason about.

[tynor]

I've noticed that as well. It's a strange behavior (bug?) in typescript. We can either do another cast, or use internal and --stripInternal.

I think using a removeChild with a Set would be easier to reason about.

Went ahead and made those changes. I agree it does look nicer, and is less intrusive.

Turns out, @internal is not required because the classes are not actually exported, only createInjector.

[nicojs]

F.y.i. I guess not having access to protected from derived sub classes is a feature:

https://www.typescriptlang.org/docs/handbook/2/classes.html#cross-hierarchy-protected-access

[tynor]

F.y.i. I guess not having access to protected from derived sub classes is a feature:

https://www.typescriptlang.org/docs/handbook/2/classes.html#cross-hierarchy-protected-access

Interesting. It seems rather unintuitive, but c'est la vie.

[nicojs]

I've added the integration test (failed on master, passed in this branch) and merged it. Great job and thanks! Released in 3.0.1