Skip to content
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.

Sign up for GitHub

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

Jump to bottom

revert: fix(core): allow toSignal in reactive contexts #52049

Closed
wants to merge 2 commits into from
Closed

revert: fix(core): allow toSignal in reactive contexts #52049

wants to merge 2 commits into from

Commits on Oct 5, 2023

  1. feat(core): create function to assert not running inside reactive con… 

    …text

Some functions or code should never run inside reactive contexts. A
function to assert that will help putting guard rails in place.
    @devversion
    devversion committed Oct 5, 2023
    Configuration menu
    Copy the full SHA
    7bc9812 View commit details
    Browse the repository at this point in the history

  2. revert: fix(core): allow toSignal in reactive contexts 

    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.
    @devversion
    devversion committed Oct 5, 2023
    Configuration menu
    Copy the full SHA
    1324534 View commit details
    Browse the repository at this point in the history