Skip to content
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

[feat] Add support for cleaning up disposable instances #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xapphire13
Copy link
Collaborator

Closes #127

cc: @mikl

@@ -36,7 +37,9 @@ export const typeInfo = new Map<constructor<any>, ParamInfo[]>();

/** Dependency Container */
class InternalDependencyContainer implements DependencyContainer {
private _registry = new Registry();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have probably done this in a separate PR. But the leading _ for private fields isn't really recommended by anyone these days. removing

Copy link
Collaborator

@MeltingMosaic MeltingMosaic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, I think we might need to add something to allow stateful factories to dispose themselves as well - for example, instanceCachingFactory won't dispose its retained instance.

@@ -540,6 +541,14 @@ export class Bar implements IBar {
}
```

# Disposable instances
All instances create by the container that implement the [`Disposable`](./src/types/disposable.ts)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spelling "created"

expect(bar.disposed).toBeTruthy();
});

it("disposes all instances of the same type", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question - what happens for array-type (@injectAll type) resolutions - do we resolve each element in the array?

}
}

it("renders the container useless", () => {
Copy link
Collaborator

@MeltingMosaic MeltingMosaic Oct 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be useful to have a UT that ensures that register() and resolve() also fail.

@microsoft microsoft deleted a comment from Xapphire13 Oct 18, 2020
@@ -360,9 +385,14 @@ class InternalDependencyContainer implements DependencyContainer {
return childContainer;
}

public dispose(): void {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be an async method.
Take for example https://www.npmjs.com/package/redis QUIT command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

container.hasInstances() or some other way to tell if instances exist without creating new ones
3 participants