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.

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

[feature request] BehaviorSubject to Signals migration #504

Open
ShacharHarshuv opened this issue Sep 18, 2024 · 8 comments
Open

[feature request] BehaviorSubject to Signals migration #504

ShacharHarshuv opened this issue Sep 18, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@ShacharHarshuv
Copy link
Contributor

ShacharHarshuv commented Sep 18, 2024

In the spirit of writing migration scripts to modernize Angular code, I want to propose the following script. Let me know if you think this is something that could fit the library.

The assumption here that everything that a BehaviorSubject is used for, a signal can be used instead, and I think it is possible to write a (relatively) robust script to do that migration. The script will:

  • Replace member instantiation of new BehaviorSubject with signal, while keeping any used generics / inferred types.
  • Replace .next with .set
  • Rename the member if it ends with $
  • Replace myValue$.value usages to myValue()
  • Replace any other myValue$ with `toObservable(myValue)
    • caveat: it might not be in an injection context. A possible solution of it will be duplicating the member in instantiation so that we'll have both myValue as a signal and myValue$ as a stream. In that case, if there is no $ as a postfix, we'll have to add it.
@jdegand
Copy link
Contributor

jdegand commented Sep 19, 2024

And convert .next to .set?

@ShacharHarshuv
Copy link
Contributor Author

Yes, sorry. Added!

@eneajaho
Copy link
Collaborator

Hi, something is coming 🙌

Let's wait a little bit more 😬

@eneajaho eneajaho added the enhancement New feature or request label Sep 19, 2024
@ShacharHarshuv
Copy link
Contributor Author

ShacharHarshuv commented Sep 19, 2024

Do you mean you are working on it, that Angular's core team is working on it, or that you think it's premature to add such a feature now?

@eneajaho
Copy link
Collaborator

I've been working on a prototype for myself to do these kinds of migration, but nothing ready yet.

@ShacharHarshuv
Copy link
Contributor Author

ShacharHarshuv commented Sep 19, 2024

Cool! I also thought that a next step could be converting something like that:

combineLatest({
  a: toObservable(this.a),
  b: toObservable(this.b),
})
.pipe(
  map(({a, b}) => this.a() + this.b()),
  distinctUntilChanged(), // ... and other operators that don't matter with signals like share etc
);

to

computed(() => this.a() + this.b());

Might be harder than I think but it feels like it could be possible.

@kbrilla
Copy link

kbrilla commented Oct 2, 2024

I would argue that this could be applyFix type of migration for VS code, but should not be used with ng migrate; Converting all BehaviorSubject signal may lead to errors, as there may by something along the line of pipe(pairwise()) somewhere in the code and using toObservable() would break this behaviour as instead, beacause of lazy nature of signals instead of abcd we would get only d delivered to pipe;

https://stackblitz.com/edit/stackblitz-starters-qnm6xx?file=src%2Fmain.ts (look at console to see difrence)
Basicly:
image
Only BehaviorSubject prints all values, while signal, and toSignal/toObservable prints only last values that were recived in CD cycle because they relay on effect().

With pairWise we would get completle diffrent values [ab], [bc], [cd], [de], [ef], [fg] compared to [dg] only

image

@ShacharHarshuv
Copy link
Contributor Author

Yeah they behave differently. Though I feel like in practice if you use a BehaviorSubject you probably only care about the last value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants