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
revert: fix(core): allow toSignal in reactive contexts #52049
Conversation
45c92c6
to
979cd60
Compare
979cd60
to
6d1d47e
Compare
…text Some functions or code should never run inside reactive contexts. A function to assert that will help putting guard rails in place.
Revert (with improvements of): dcf18dc We recently landed a change that allows `toSignal` to be called from within reactive contexts (e.g. `effect`/`computed`). After more thorough investigatio and consideration with the team, we feel like allowing `toSignal` to be called in such contexts is encouraging non-ideal / hard-to-notice code patterns. e.g. a new subscription to an observable is made every time `toSignal` is invoked. There is no caching done here. Additionally, multiple new subscriptions can trigger unintended side-effects- that may slow down the app, result in incorrect/unexpected behavior or perform unnecessary work. Users should instead move the `toSignal` call outside of the `computed` or `effect` and then read the signal values from within their `computed`. e.g. ```ts computed(() => { const smth = toSignal(coldObservable$) return smth() + 2; } ``` --> should instead be: ```ts const smth = toSignal(coldObsverable$); computed(() => smth() + 2); ``` In cases where a new subscription for each invocation is actually intended, a manual subscription can be made. That way it's also much more obvious to users that they are triggering side-effects every time, or causing new subscriptions.
6d1d47e
to
1324534
Compare
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.
LGTM
Reviewed-for: public-api
Reviewed-for: fw-core
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.
reviewed-for: public-api
Caretaker note: I cannot remove pending reviewers. please ignore. PR has sufficient approvals |
This PR was merged into the repository by commit ced66d4. |
Revert (with improvements of): dcf18dc We recently landed a change that allows `toSignal` to be called from within reactive contexts (e.g. `effect`/`computed`). After more thorough investigatio and consideration with the team, we feel like allowing `toSignal` to be called in such contexts is encouraging non-ideal / hard-to-notice code patterns. e.g. a new subscription to an observable is made every time `toSignal` is invoked. There is no caching done here. Additionally, multiple new subscriptions can trigger unintended side-effects- that may slow down the app, result in incorrect/unexpected behavior or perform unnecessary work. Users should instead move the `toSignal` call outside of the `computed` or `effect` and then read the signal values from within their `computed`. e.g. ```ts computed(() => { const smth = toSignal(coldObservable$) return smth() + 2; } ``` --> should instead be: ```ts const smth = toSignal(coldObsverable$); computed(() => smth() + 2); ``` In cases where a new subscription for each invocation is actually intended, a manual subscription can be made. That way it's also much more obvious to users that they are triggering side-effects every time, or causing new subscriptions. PR Close #52049
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…text (angular#52049) Some functions or code should never run inside reactive contexts. A function to assert that will help putting guard rails in place. PR Close angular#52049
Revert (with improvements of): dcf18dc We recently landed a change that allows `toSignal` to be called from within reactive contexts (e.g. `effect`/`computed`). After more thorough investigatio and consideration with the team, we feel like allowing `toSignal` to be called in such contexts is encouraging non-ideal / hard-to-notice code patterns. e.g. a new subscription to an observable is made every time `toSignal` is invoked. There is no caching done here. Additionally, multiple new subscriptions can trigger unintended side-effects- that may slow down the app, result in incorrect/unexpected behavior or perform unnecessary work. Users should instead move the `toSignal` call outside of the `computed` or `effect` and then read the signal values from within their `computed`. e.g. ```ts computed(() => { const smth = toSignal(coldObservable$) return smth() + 2; } ``` --> should instead be: ```ts const smth = toSignal(coldObsverable$); computed(() => smth() + 2); ``` In cases where a new subscription for each invocation is actually intended, a manual subscription can be made. That way it's also much more obvious to users that they are triggering side-effects every time, or causing new subscriptions. PR Close angular#52049
We recently landed a change that allows
toSignal
to be called from within reactive contexts (e.g.effect
/computed
). After more thorough investigatio and consideration with the team, we feel like allowingtoSignal
to be called in such contexts is encouraging non-ideal / hard-to-notice code patterns.e.g. a new subscription to an observable is made every time
toSignal
is invoked. There is no caching done here. Additionally, multiple new subscriptions can trigger unintended side-effects- that may slow down the app, result in incorrect/unexpected behavior or perform unnecessary work.Users should instead move the
toSignal
call outside of thecomputed
oreffect
and then read the signal values from within theircomputed
. e.g.--> should instead be:
In cases where a new subscription for each invocation is actually intended, a manual subscription can be made. That way it's also much more obvious to users that they are triggering side-effects every time, or causing new subscriptions.