Prototype of the RxJS interop layer for signals #49154
Conversation
angular-robot
bot
added
detected: feature
|
|
||
| it('should produce an observable that tracks a signal', async () => { | ||
| const counter = signal(0); | ||
| const counter$ = fromSignal(counter).pipe(take(3), toArray()).toPromise(); |
There was a problem hiding this comment.
A bit unusual to use the $ suffix on a promise. Besides, toPromise() is deprecated in RxJS7.
There was a problem hiding this comment.
Yep, good catch - fixed.
Angular's on RxJS 6 though, so no fancy latestValueFrom for us.
There was a problem hiding this comment.
What stoping you from moving to a higher version of Rxjs?
There was a problem hiding this comment.
Google uses RxJS 6 internally, so Angular needs to build with it.
We could (and probably will) upgrade to using 7, but we'd still need to be compatible with 6, so it's not been a priority.
alxhub
force-pushed
the
fw/reactivity/rxjs-interop/first
branch
3 times, most recently
from
4730e8e
to
d94877e
Compare
|
can I remove zone when I use Signal？ |
There was a problem hiding this comment.
LGTM for dev-infra
Note: this will require us to create this package on npm prior to being able to perform a release after this pull request lands.
@alxhub I also wanted to ask, why is this not in core as well? It seems to me that it makes more sense to have this colocated with signals rather than ths which feels a little like an indirection.
pullapprove
bot
requested review from
dylhunn,
jessicajaniuk and
pkozlowski-opensource
|
Hey @alxhub! Nice work! But why Signal type dont just implement the InteropObservable type to build the bridge between rxjs and signals? Is there a reason? Think how the usage sounds better: combineLatest([myPrimitiveSignal, myObservable$]) // ...and so on! |
packages/rxjs-interop/PACKAGE.md
Outdated
| @@ -0,0 +1 @@ | |||
| This package includes utilities related to using the RxJS library in conjunction with Angular's signal-based reactivity system. | |||
There was a problem hiding this comment.
Do we want to mention signals explicitly here? I guess long-term we want to put other non-signal, RxJS related stuff here.
packages/rxjs-interop/README.md
Outdated
| Angular RxJS Interop | ||
| ========= | ||
|
|
||
| This package includes utilities related to using the RxJS library in conjunction with Angular's signal-based reactivity system. |
There was a problem hiding this comment.
Do we want to mention signals / reactivity explicitly here? I guess long-term we want to put other non-signal, RxJS related stuff here.
packages/rxjs-interop/package.json
Outdated
| "peerDependencies": { | ||
| "@angular/core": "0.0.0-PLACEHOLDER", | ||
| "rxjs": "^6.5.3 || ^7.4.0", | ||
| "zone.js": "~0.11.4 || ~0.12.0" |
There was a problem hiding this comment.
Do we need explicit peer dep on zone.js?
There was a problem hiding this comment.
@alxhub Can you explain please this part please?
I mean it was a rxjs-interop package, probably the plan was @angular/rxjs-interop.
Now as I see, rxjs-interop will be the part of core, so zone.js is still will be a dependency there. As I heard, signals are advertised as zoneless solutions, which is currently the only (main) reason for me why we should use them.
I would like to understand that this pull request is just a step forward or there are other reason why we still need zone.js.
I think having another npm package as @angular/rxjs-interop or something is better until zone.js cannot be eliminated from core. In long term I agree, it should be in core library.
| * | ||
| * @developerPreview | ||
| */ | ||
| export function fromObservable<T, U>(source: Observable<T>, initialValue: U): Signal<T|U>; |
There was a problem hiding this comment.
I find it surprising that the type of the signal and initial state are different. What is the use-case that we are targeting here?
There was a problem hiding this comment.
My guess is that it's mainly useful when you have an Observable<Something> and you want null or undefined as default value.
There was a problem hiding this comment.
implementation of rxjs firstValueFrom and lastValueFrom also have different type for defaultValue.
But maybe instead of initialValue rxjs defaultValue is more fitting as it is not only initial - so in a state of suspense but also if observable completed without any value too. Also keeping rxjs config approach can be useful in future if we would like to extend this config,
export interface FirstValueFromConfig<T> {
defaultValue: T;
}
export function firstValueFrom<T, D>(source: Observable<T>, config: FirstValueFromConfig<D>): Promise<T | D>;
export function firstValueFrom<T>(source: Observable<T>): Promise<T>;
Then again If the source Observable does not complete or emit, you will end up with a Promise that is hung up, and potentially all of the state of an async function hanging out in memory,
So they differentiate those two states and do not throw nor return a default value if there is no emission before read.
Initial value could also be achieved by using startWith operator but that would not work if the subsequent emission would be empty.
There was a problem hiding this comment.
it's mainly useful when you have an Observable and you want null or undefined as default value.
But this is strange since having null as a default value means that anyone can read this value so it should be legitimate type of a signal. In this case it would be T | null
There was a problem hiding this comment.
Defaulting to null | undefined instead of initialValue: U would be | async pipe | null problem again
There was a problem hiding this comment.
Just throwing in my 2 cents here...
RxJS doesn't distinguish between observables that emit immediately vs asynchronously. So I don't think consumers of observables should rely on knowledge about when they're going to emit. This could cause unexpected errors. Much better would be to type it as T | null always, but optimally let devs pass in a type argument (or infer from initial state passed in) if they happen to know 100% that the observable emits immediately. But in my opinion even this explicit type parameter is dangerous because you could modify the observable upstream and TypeScript could not prevent a runtime error. And I don't see a significant difference between null and undefined here, but I haven't thought that through carefully.
The only way safe way out of this TypeScript awkwardness is if the observables themselves contained knowledge about whether they are going to emit immediately or not.
There was a problem hiding this comment.
I was thinking about this before, not sure if its possible for the given use case, but you could introduce a 'loading' object which is static and use it as a type. That way you could have a difference between loading, null, undefined and T.
What you would have to do if handle then that type in every pipe in angular etc. Maybe a overkill but just wanted to point the idea out.
|
|
||
| // TODO(alxhub): subscription cleanup logic | ||
|
|
||
| return computed(() => { |
There was a problem hiding this comment.
Why is it computed? I guess there are 2 things we need to do:
- make sure that it is not mutable;
- it is branded (?)
Normally a simple function would be enough, I guess this is about branding?
There was a problem hiding this comment.
What dose it mean 'branded'?
alxhub
force-pushed
the
fw/reactivity/rxjs-interop/first
branch
2 times, most recently
from
7ae3be6
to
377013b
Compare
alxhub
force-pushed
the
fw/reactivity/rxjs-interop/first
branch
2 times, most recently
from
baf1ef3
to
b833473
Compare
alxhub
added
action: merge
alxhub
force-pushed
the
fw/reactivity/rxjs-interop/first
branch
from
b833473
to
78fa475
Compare
We've been experimenting with the DeepReadonly type that would make signal values deeply read-only and prevent accidental changes without going to the owner of data. What we've found out during the experiments is that additional safety net has more drawbacks than benefits: it just introduces too much friction to be practical for daily usage.
| case StateKind.Error: | ||
| throw current.error; | ||
| case StateKind.NoValue: | ||
| throw new Error(`fromObservable() signal read before the Observable emitted`); |
There was a problem hiding this comment.
nit: should it be RuntimeError?
There was a problem hiding this comment.
nit: TODO is left in. This needs a runtime error code.
| effect(() => { | ||
| seenValue = counter(); | ||
| }); | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
thinking out loud: those tests will fail when we change the timing of effects, right? Which kind of rises the question of testing patterns for effects. A separate discussion that we should have.
There was a problem hiding this comment.
As an alternative we could have written those tests with just a signal and / or computed and just assert that new value is available. We might leave a test or two with effect but IMO ones based on just signals would be more readable.
There was a problem hiding this comment.
LGTM
Left a set of bunch of nit / "thinking out loud" comments but nothing that would block this PR. We will need to coordinate this PR landing with other in-flight ones as we will have conflicts around DeepReadonly and timing of effects.
Reviewed-for: public-api
Reviewed-for: fw-core
This commit adds the infrastructure for `@angular/core/rxjs-interop`, a new core entrypoint which provides bidirectional interoperability between Angular's built-in reactivity system of synchronous signals, and the RxJS `Observable` abstraction. The new entrypoint is set up as an empty shell in this commit, with its implementation to follow in a followup.
This commit adds the basic sketch for the implementation of `fromObservable` and `fromSignal`, the two basic primitives which form the RxJS interop layer with signals.
This commit implements an RxJS operator `takeUntilDestroyed` which terminates an Observable when the current context (component, directive, etc) is destroyed. `takeUntilDestroyed` will inject the current `DestroyRef` if none is provided, or use one provided as an argument.
alxhub
force-pushed
the
fw/reactivity/rxjs-interop/first
branch
from
78fa475
to
877d100
Compare
| case StateKind.Error: | ||
| throw current.error; | ||
| case StateKind.NoValue: | ||
| throw new Error(`fromObservable() signal read before the Observable emitted`); |
There was a problem hiding this comment.
nit: TODO is left in. This needs a runtime error code.
|
This PR was merged into the repository by commit e883198. |
This commit adds the infrastructure for `@angular/core/rxjs-interop`, a new core entrypoint which provides bidirectional interoperability between Angular's built-in reactivity system of synchronous signals, and the RxJS `Observable` abstraction. The new entrypoint is set up as an empty shell in this commit, with its implementation to follow in a followup. PR Close #49154
This commit implements an RxJS operator `takeUntilDestroyed` which terminates an Observable when the current context (component, directive, etc) is destroyed. `takeUntilDestroyed` will inject the current `DestroyRef` if none is provided, or use one provided as an argument. PR Close #49154
If you already decided on it, why are you discussing it in RFC? |
|
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. |
For context, see this Github Discussion on our experiments with fine-grained reactivity in Angular
(the first commit here is #49150 which serves as a base for this implementation)
This is the prototype implementation of the RxJS interop layer, sketched out as a separate package (
@angular/rxjs-interop).