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

Async pipe default value #47608

Conversation

pkozlowski-opensource
Copy link
Member

No description provided.

This tiny refactoring removes the unnecessary curly brackets that were wrapping
the entire async pipe tests. This was creating additional indentation.
The diff of this change looks big but the actual change boils down to
removal of curly brackets.
Previously the async pipe transform method was recursively calling
itself (in case of observable / promise reference change) making
the code flow hard to follow. This change removes the self-call and
has a single return statement from the transform method.
@angular-robot angular-robot bot added the feature Issue that requests a new feature label Oct 3, 2022
@pkozlowski-opensource pkozlowski-opensource added the area: common Issues related to APIs in the @angular/common package label Oct 3, 2022
@ngbot ngbot bot added this to the Backlog milestone Oct 3, 2022
}

return this._latestValue;
return this._latestValue == null ? defaultValue : this._latestValue;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: should we always apply the defaultValue to the emitted null / undefined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take: no, and in fact we should call the default value initialValue instead, and only apply it before the Observable emits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was always intended that way.

Call the default value initialValue instead, and only apply it before the Observable emits.

@@ -107,23 +107,26 @@ export class AsyncPipe implements OnDestroy, PipeTransform {
// TypeScript has a hard time matching Observable to Subscribable, for more information
// see https://github.com/microsoft/TypeScript/issues/43643

transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, defaultValue: T): T;
transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>): T|null;
transform<T>(obj: null|undefined): null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: should we add defaultValue overload here as well?

@eneajaho
Copy link
Contributor

eneajaho commented Oct 3, 2022

Hi,
great addition, I just wanted to know what is the benefit (dx point of view) of this:

 <test-cmp [user]="obs$ | async: { id: 1 }"></test-cmp>

over:

 <test-cmp [user]="(obs$ | async) ?? { id: 1 }"></test-cmp>
or
 <test-cmp [user]="(obs$ | async) || { id: 1 }"></test-cmp>

The default value on pipe is going to use the pipe syntax ":", while ?? and || are javascript syntax.

if (!this._obj) {
if (obj) {
this._subscribe(obj);
transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>|null|undefined, defaultValue = null): T
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: should we default defaultValue to null or undefined?

Relevant discussion in #43727 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - null should be the "default default" (or really, the default initial value) - basically the current behavior of the pipe if not provided. That's more of an implementation detail, though.

@pkozlowski-opensource
Copy link
Member Author

@eneajaho good question - the way I see it: the answer will largely depend on #47608 (comment) - if we want to have the "apply the default value on subscribe" semantic I guess we will need an additional argument to the async pipe.

This change makes it possible to specify a default value for
the async pipe. The default value is returned in the two cases:
- observable / promise didn't emit yet;
- observable / promise emitted null / undefined.
@alxhub
Copy link
Member

alxhub commented Oct 3, 2022

Another interesting question: do we actually want "default value" semantics? Or would a better mechanic be some ability to say "I promise that this Observable always emits synchronously, and will therefore always have a value"

@JoostK
Copy link
Member

JoostK commented Oct 3, 2022

Another interesting question: do we actually want "default value" semantics? Or would a better mechanic be some ability to say "I promise that this Observable always emits synchronously, and will therefore always have a value"

I would certainly prefer a "sync" async pipe over a default/initial value; it may be non-trivial to create a default value of type T to get rid of the null type, and an initial value would be meaningless for observables that emit synchronously upon subscription.

@kbrilla
Copy link

kbrilla commented Oct 3, 2022

what about

 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, {synch: true}): T;
 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, {initialValue: T}): T;

// and maybe 
 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, {defaultValue: T}): T;
 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, { {initialValue: T; defaultValue: T}): T;

If I remember correctly though using objects in pipes arguments is not the best idea so maybe

 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, synch: true): T;
 transform<T>(obj: Observable<T>|Subscribable<T>|Promise<T>, synch:  false, initialValue: T): T;

where synch would tell that observable is able to emit synchronously as mentioned by @alxhub

Another interesting question: do we actually want "default value" semantics? Or would a better mechanic be some ability to say "I promise that this Observable always emits synchronously, and will therefore always have a value"

initialValue would be used only until first emit thus it should not be able to provide both synch: true and initialValue

defaultValue would act as ?? but I think that leaving that out of the async pipe and using javascript syntax is better as mentioned by @eneajaho

 <test-cmp [user]="(obs$ | async) ?? { id: 1 }"></test-cmp>
or
 <test-cmp [user]="(obs$ | async) || { id: 1 }"></test-cmp>

doing it this way would provide the way to provide initial value to observables that do not emit synchronously and let async pipe know when it does and it's safe to drop null from the return type

@pkozlowski-opensource
Copy link
Member Author

Another interesting question: do we actually want "default value" semantics? Or would a better mechanic be some ability to say "I promise that this Observable always emits synchronously, and will therefore always have a value"

I would certainly prefer a "sync" async pipe over a default/initial value; it may be non-trivial to create a default value of type T to get rid of the null type, and an initial value would be meaningless for observables that emit synchronously upon subscription.

where synch would tell that observable is able to emit synchronously as mentioned by @alxhub

@alxhub @JoostK @kbrilla I hear the sentiment / idea for the "this observable is sync" idea. But I don't think we should go down this path with the current async pipe, for the following reasons:

  • the async pipe, as of today, works with promises as well, and there " observable is able to emit synchronously" idea doesn't fit;
  • it is way too easy to apply a transformation to an observable, somewhere in the chain, that would flip it from "sync" to "async"

Having the sync: true is similar to having myVar! - it is user-override of what the system knows about the values. It might be needed in some cases but can very well start breaking during runtime. So personally I'm not fan of this approach. In this sense the "initial value" is a safer option.

Another idea we can consider is creating a separate pipe that wouldn't handle promises and could deal only with sync-emitting observables. But this would fragment the current ecosystem.

@kbrilla
Copy link

kbrilla commented Oct 4, 2022 via email

@johngrimsey
Copy link

🍿 👏

@pkozlowski-opensource
Copy link
Member Author

Closing based on the discussion in #43727 (comment)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: common Issues related to APIs in the @angular/common package common: async pipe feature Issue that requests a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants