-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(ISubscription): update type definition of ISubscription::closed #2249
Conversation
@@ -13,7 +13,7 @@ export type TeardownLogic = AnonymousSubscription | Function | void; | |||
|
|||
export interface ISubscription extends AnonymousSubscription { | |||
unsubscribe(): void; | |||
closed: boolean; | |||
readonly closed: boolean; |
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.
👍 this will encourage TypeScript users to not try to modify this value (as it shouldn't be mutable)
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.
@david-driscoll check #2234 as well, this PR is subset of it only updated type definition, while it does updates actual code behaviors.
Technically, a LOT of the codebase returns subclasses of I guess what I'm saying is: Hold off on that refactor, there's likely one coming anyhow... especially with the planned scheduling changes. |
LGTM |
LGTM |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description:
Originated from #2234, doesn't add any code changes but only update type definition for interface clarity.
Currently it's not so benefitable even for TS user since all of codebase just return
Subscription
everywhere instead of using interfaces, that need to be dealt with separate refactoring.Related issue (if exists):
#2234