-
-
Notifications
You must be signed in to change notification settings - Fork 4
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 finalizer #93
base: main
Are you sure you want to change the base?
Conversation
|
I'm not sure what the major change is.
|
thanks @godu I will carefully review it and diside if it should be major. |
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.
Hey @godu , I like this change, but please consider my comments
(config) => | ||
Effect.acquireRelease(Effect.succeed(new AccountClient(config)), (client) => | ||
Effect.sync(() => client.destroy()), | ||
), | ||
); | ||
|
||
/** |
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.
We can use Layer.scoped when constructing instance layer
export const AccountClientInstanceLayer = Layer.scoped(
AccountClientInstance,
makeAccountClientInstance,
);
in this way we can avoid introducing breaking change
additionally we can add AccountClientInstanceLayerScoped
and defaultLayerScoped
and other pairs, it is normal practice in effect ecosystem. if you want to control scope you use scoped
version of layer.
Destroy client after layer lifecycle to release idle connections.