fix(Subscription): Limit Subscription::closed public accessibility to readonly #2234
Conversation
| rxTestScheduler.flush(); | ||
|
|
||
| expect((schedulerMock as any).schedule).calledOnce; |
There was a problem hiding this comment.
while looking codes I noticed this test case actually did not assert anything, modified to have assertion but not sure if this is intended one - will update accordingly as suggested.
kwonoj
force-pushed
the
fix-subscription-closed
branch
from
0ef4d83 to
0537a32
Compare
kwonoj
changed the title
… readonly BREAKING CHANGE: Cannot change isClosed state without unsubscribe()
kwonoj
force-pushed
the
fix-subscription-closed
branch
from
0537a32 to
b64e50c
Compare
|
I'm not sure why this change is necessary. How does it effect perf? |
|
To clarify, we live in a world where |
|
Way I'm seeing this change is enhancing ergonomics with small complexity addition in codebases. It is indeed this codebase is javascript and anyone can actually do anything if they try to avoid given interfaces, on the other hand if library's interface prevents any accidental incorrect usage, it would be better to prevent user's confusion. Perfwise, previously it is just property access and with this change it'll be getter function, If this change doesn't seems give sufficient benefit to library users, I'd like at least propose to update signature of |
|
Honestly, @kwonoj, I think I'm 👎 on this change and other changes like it. I'm happy with TypeScript enforcement (and possible _name "enforcement" in "private" or "protected" variables cases) for now. I'm willing to be convinced otherwise, of course. But right now I'm not feeling it. |
|
@Blesh fine for me. Just one thing, can we add this as agenda for next meeting about how much of runtime guarantee / validation we'd like to provide? Currently codebase barely does not provide anything on it, and this PR is kind of those to adding some more soundness on runtime (by introducing runtime complexity). As you can see, there are some of opened issues to demand certain level of runtime validation which non-TS user could benefit. User of TS this doesn't be too much issues for myself, but worth to think of where to draw those lines (such as it's already good as-is vs. we need something more) Closing PR without checking it. |
|
@kwonoj that's fine. I'm happy to discuss it. I'm on the side of: "If you want soundness, use a type system, otherwise this is JavaScript and I'll duck-punch all the things I want" Although, I really would like to go through and rename all private and protected variables to have underscores before them. That's been bothering me. |
: That maybe sufficient soundness of runtime level we'd like to achieve. I think having settled down line would be great whatever level we'd like to have. |
|
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:
This PR changes the signature of
ISubscription::closedtoreadonly.Currently, it is allowed to change state of
closedas well asunsubscribe()but those two are causing different outcome byunsubscribeactually does necessary teardown for unsubscription, whileclosedflag only changes state flag. Instead, this PR explicitly makes given flag as getter only to read state, otherwise let useunsubscribe()if needed.This is indeed breaking change if anyone has been using
closedto control subscription state, it won't work anymore. For now, I have targeted this to master until gets reviewed and approved.BREAKING CHANGE: Cannot change closed state without unsubscribe()
Related issue (if exists):