-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
lib: consolidate Channel and ActiveChannel into single class #57977
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57977 +/- ##
=======================================
Coverage 90.27% 90.27%
=======================================
Files 630 630
Lines 186165 186130 -35
Branches 36485 36477 -8
=======================================
- Hits 168064 168037 -27
Misses 10974 10974
+ Partials 7127 7119 -8
🚀 New features to boost your workflow:
|
|
@gurgunday this was intentionally separated for performance reasons. Now it has to do the checks that it formerly could just skip. It's probably not a huge overhead but it should be measurable. |
|
I see, are you down to merging if there are no regressions? I'm not really convinced a boolean check is slower than changing the prototype dynamically |
| #maybeDeactivate() { | ||
| if (this.#active && !this.#subscribers.length && !this.#stores.size) { | ||
| this.#active = false; | ||
| this.#subscribers = undefined; |
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 changes this._subscribers to this.#subscribers which is a breaking change.
|
Added a benchmark that tests various scenarios |
|
I would very much want to see benchmark results before accepting this change. As @BridgeAR said, this was intentionally designed this way. The reason being because it shifts the cost to occur only when a subscribe/unsubscribe happens which is far less frequent than a publish or runStores. It is specifically designed to do the least amount of work at publish time as possible to allow it to be called liberally without worry about the cost, especially when there are no subscribers. When I wrote this initially, I tested several different structures including an internally-switched model like this. The prototype switch was the fastest by far, it was not even close. There are also some changes here beyond just the internal-switching structure that I would be concerned about: private fields, last I checked, were still notably slower, and object destructuring is also slower than the manual extraction it was doing previously. |
Merges the Channel and ActiveChannel classes
I think dynamically modifying prototypes is not good practice and leads to unnecessary duplication
We can simply use a single class with an internal state
It also replaces the good ol'
_properties with private propertiesWe should be able to get this into v24