feat(common): allow any Subscribable in async pipe #39627

divdavem · 2020-11-10T17:32:00Z

As only methods from the Subscribable interface are currently used in the implementation of the async pipe, it makes sense to make it explicit so that it works successfully (regarding type-checking) with any other implementation instead of only Observable.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

The following component does not compile successfully:

import { Component, Input } from '@angular/core';
import { Subscribable } from 'rxjs';

@Component({
  selector: 'hello',
  template: `Hello {{ nameSubscribable | async }}`
})
export class HelloComponent  {
  @Input() nameSubscribable: Subscribable<any>;
  // Note that it works fine when replacing the above line with:
  // @Input() nameSubscribable: any;
}

Here is the compilation error:

No overload matches this call.
The last overload gave the following error.
Argument of type 'Subscribable<any>' is not assignable to parameter of type 'Promise<unknown>'.
Type 'Subscribable<any>' is missing the following properties from type 'Promise<unknown>': then, catch, [Symbol.toStringTag], finally

(cf https://stackblitz.com/edit/angular-ivy-fypzxs?file=src/app/hello.component.ts )

What is the new behavior?

The compilation of the previous component is successful.

Does this PR introduce a breaking change?

Yes
No

Other information

The important part of this PR is about changing the input type from Observable to Subscribable in async pipe.
For consistency, I have renamed some local variables to contain subscribable instead of observable.
In packages/core/src/util/lang.ts, I have added the isSubscribable function that does the same thing as isObservable but, in case the TODO mentioned in isObservable is implemented those two methods would be different.

petebacondarwin · 2020-11-12T08:44:05Z

This is an interesting idea, but I want to check with the rest of the team before making any decision on whether to go ahead with this.

packages/core/src/util/lang.ts

atscott · 2020-11-12T22:09:07Z

This is an interesting idea, but I want to check with the rest of the team before making any decision on whether to go ahead with this.

Agreed that this seems totally reasonable and straightforward. Because it actually expands the type rather than narrowing it, I believe this is a non-breaking change (triggered presubmit as a sanity check) and I can't think of any good reason not to move forward with it. I suppose there's a question as to why this change is necessary since I can't say I've ever encountered anyone asking for this feature (other than now).

atscott

As expected, the presubmit doesn't have any failures related to this change. I'm in favor of moving this forward. As I said before, I can't think of any reason to not allow Subscribable since it really is already supported as the pipe is implemented; the type just needs to accurately reflect the wider range of supported inputs.

IgorMinar

This change looks good to me from the API perspective. Thanks

Reviewed-for: public-api

gkalpak

LGTM for dev-infra

Reviewed-for: dev-infra

packages/core/src/util/lang.ts

packages/common/test/pipes/async_pipe_spec.ts

AndrewKushnir

Reviewed-for: public-api

FYI, I've also added the "cleanup" label since there is a merge conflict with the master branch. Feel free to remove it once the conflict is resolved. Thank you.

As only methods from the Subscribable interface are currently used in the implementation of the async pipe, it makes sense to make it explicit so that it works successfully with any other implementation instead of only Observable.

divdavem · 2020-11-20T10:29:57Z

@AndrewKushnir @gkalpak @IgorMinar @atscott Thank you for your review 👍

I do not have the ability to remove the cleanup label, but I have rebased the PR to solve the conflict with the master branch and done a few changes as requested by @gkalpak
I suppose the PR is ready to be merged, unless you have any other comment.

petebacondarwin

Reviewed-for: public-api

AndrewKushnir · 2020-11-21T02:09:09Z

Presubmit + Global Presubmit.

angular-automatic-lock-bot · 2020-12-24T16:46:27Z

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.}

google-cla bot added the cla: yes label Nov 10, 2020

gbumanzordev approved these changes Nov 11, 2020

View reviewed changes

divdavem force-pushed the asyncPipeType branch from 639bee3 to 1a952a8 Compare November 12, 2020 08:35

petebacondarwin added the state: needs eng input label Nov 12, 2020

petebacondarwin reviewed Nov 12, 2020

View reviewed changes

packages/core/src/util/lang.ts Outdated Show resolved Hide resolved

divdavem marked this pull request as ready for review November 12, 2020 08:57

pullapprove bot requested review from atscott and gkalpak November 12, 2020 08:58

AndrewKushnir added the cross-cutting: types label Nov 12, 2020

atscott mentioned this pull request Nov 13, 2020

refactor(core): remove isObservable TODO #39669

Closed

atscott approved these changes Nov 13, 2020

View reviewed changes

IgorMinar approved these changes Nov 13, 2020

View reviewed changes

atscott added the target: minor This PR is targeted for the next minor release label Nov 13, 2020

gkalpak approved these changes Nov 19, 2020

View reviewed changes

packages/core/src/util/lang.ts Show resolved Hide resolved

packages/common/test/pipes/async_pipe_spec.ts Outdated Show resolved Hide resolved

packages/common/test/pipes/async_pipe_spec.ts Show resolved Hide resolved

pullapprove bot requested review from alxhub, AndrewKushnir, atscott and jelbourn November 19, 2020 18:00

AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 19, 2020

AndrewKushnir approved these changes Nov 19, 2020

View reviewed changes

pullapprove bot requested a review from petebacondarwin November 19, 2020 21:52

divdavem force-pushed the asyncPipeType branch from 1a952a8 to bf34062 Compare November 20, 2020 09:50

feat(common): allow any Subscribable in async pipe

8da7b7e

As only methods from the Subscribable interface are currently used in the implementation of the async pipe, it makes sense to make it explicit so that it works successfully with any other implementation instead of only Observable.

divdavem force-pushed the asyncPipeType branch from bf34062 to 8da7b7e Compare November 20, 2020 10:01

gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 20, 2020

petebacondarwin approved these changes Nov 20, 2020

View reviewed changes

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Nov 20, 2020

AndrewKushnir removed request for atscott, jelbourn and alxhub November 23, 2020 06:02

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 23, 2020

AndrewKushnir closed this in c7f4abf Nov 23, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 24, 2020

pullapprove bot added area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) comp: docs/api labels Dec 24, 2020

ngbot bot modified the milestone: Backlog Dec 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): allow any Subscribable in async pipe #39627

feat(common): allow any Subscribable in async pipe #39627

divdavem commented Nov 10, 2020

petebacondarwin commented Nov 12, 2020

atscott commented Nov 12, 2020

atscott left a comment

IgorMinar left a comment

gkalpak left a comment

AndrewKushnir left a comment

divdavem commented Nov 20, 2020

petebacondarwin left a comment

AndrewKushnir commented Nov 21, 2020 •

edited

angular-automatic-lock-bot bot commented Dec 24, 2020

feat(common): allow any Subscribable in async pipe #39627

feat(common): allow any Subscribable in async pipe #39627

Conversation

divdavem commented Nov 10, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petebacondarwin commented Nov 12, 2020

atscott commented Nov 12, 2020

atscott left a comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

divdavem commented Nov 20, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Nov 21, 2020 • edited

angular-automatic-lock-bot bot commented Dec 24, 2020

AndrewKushnir commented Nov 21, 2020 •

edited