proposal-signals / signal-polyfill

Implementation tracking the current state of https://github.com/tc39/proposal-signals
Apache License 2.0
185 stars 13 forks source link

Batch unwatch test issue fixed #24

Closed Roeck closed 1 month ago

Roeck commented 2 months ago

Changes made:

Yes, the wrapper.ts had some alterations for passing this specific test, but they also ensure that the core functionality works correctly

support batch unwatch

littledan commented 2 months ago

Does this change do the same as #26? Should it be landed?

ds300 commented 2 months ago

@ds300 Just to clarify, I've tested these changes thoroughly, and they don't introduce any new issues. These sanity checks are not meant to hide real errors but to prevent the feature from breaking when it encounters unusual situations.

@Roeck continuing here to avoid clogging up the other PR with discussion about at this one.

Your PR makes it so that legitimate (if very unlikely) errors would be silently ignored. But that's not what we want. If any of those sub conditions in your if condition are false then it's an error in the core signals library and we should throw. You could change them to assertions that throw, and that would be ok to merge and keep during this prototyping phase, but ultimately those are things that can and should be tested by unit tests rather than sanity checks at run time.

littledan commented 1 month ago

Landed https://github.com/proposal-signals/signal-polyfill/pull/26 instead