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

Jump to bottom

Fix AsyncPipe types for RxJS 6 and 7 #41590

Closed
wants to merge 1 commit into from
Closed

Fix AsyncPipe types for RxJS 6 and 7 #41590

wants to merge 1 commit into from

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented Apr 13, 2021 •
edited

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) this is a non-breaking change and adding a test would require adding a development dependency to this project of rxjs@next, which seems like overkill. However this will get exercised extremely well in google3.
  • Docs have been added / updated (for bug fixes / features) unnecessary

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:

This resolves an issue reported to me by @leggechr and escalated to @alxhub. The issue is discussed here as well: microsoft/TypeScript#43643

What is the current behavior?

Currently, when upgrading to RxJS 7, in many cases Angular templates are unable to infer the proper value type from RxJS observables. This is because of the TypeScript issue/quirk pointed out above.

What is the new behavior?

The value types will be inferred properly in the Angular template.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This is affecting targets within google3 and I recommend you work with @leggechr and escalate this as a Googler issue.

@google-cla google-cla bot added the cla: yes label Apr 13, 2021
@pullapprove pullapprove bot requested a review from mhevery April 13, 2021 00:19
@zarend zarend added the area: common Issues related to APIs in the @angular/common package label Apr 13, 2021
@ngbot ngbot bot added this to the Backlog milestone Apr 13, 2021
alxhub
alxhub requested changes Apr 13, 2021
View reviewed changes
packages/common/src/pipes/async_pipe.ts Outdated Show resolved Hide resolved
packages/common/src/pipes/async_pipe.ts Outdated Show resolved Hide resolved
@benlesh
fix(core): AsyncPipe now compatible with RxJS 7
679fb71 
Adds a fix to make sure that RxJS v7 Observable is compatible with AsyncPipe. This is a typings-only change.
For more information see: microsoft/TypeScript#43643
@benlesh benlesh force-pushed the fix_async-pipe-types-for-RxJS-7 branch from 29665eb to 679fb71 Compare April 13, 2021 17:43
@jelbourn jelbourn mentioned this pull request Apr 30, 2021
Closed
@e-oz
Copy link

e-oz commented May 1, 2021

Sorry if it's a noobish request, but could one of you help bots to complete the checks? Please.
I'd like to try RxJS 7 with the Angular :)

petebacondarwin
petebacondarwin approved these changes May 1, 2021
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LOL! We only just recently switched this from Observable to Subscribable!

I am not sure where this puts us in terms of breaking changes... it is possible that, since we made the change, someone has implemented this pipe with a Subscribable object rather than an Observable, which would no longer work. But I suspect this is very unlikely.

Approving generally. This should trigger a public-api review round.

@petebacondarwin petebacondarwin added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels May 1, 2021
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label May 1, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@alxhub
Copy link
Member

alxhub commented May 3, 2021

I am not sure where this puts us in terms of breaking changes... it is possible that, since we made the change, someone has implemented this pipe with a Subscribable object rather than an Observable, which would no longer work. But I suspect this is very unlikely.

Since this change only adds Observable<T> to the signature and doesn't remove Subscribable<T>, such a usage should still work.

alxhub
alxhub approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from petebacondarwin May 3, 2021 21:33
AndrewKushnir
AndrewKushnir approved these changes May 3, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from jessicajaniuk May 3, 2021 22:03
jessicajaniuk
jessicajaniuk approved these changes May 3, 2021
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

I Observe that these changes look good.

reviewed-for: public-api

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels May 3, 2021
@AndrewKushnir
Copy link
Contributor

FYI, presubmit looks good, the changes were approved (and the changes look backwards-compatible), so I'm adding the "merge" label. Thank you.

@mhevery mhevery closed this in e387d22 May 4, 2021
mhevery pushed a commit that referenced this pull request May 4, 2021
@benlesh @mhevery
fix(core): AsyncPipe now compatible with RxJS 7 (#41590)
9759bca 
Adds a fix to make sure that RxJS v7 Observable is compatible with AsyncPipe. This is a typings-only change.
For more information see: microsoft/TypeScript#43643

PR Close #41590
mhevery pushed a commit that referenced this pull request May 4, 2021
@benlesh @mhevery
fix(core): AsyncPipe now compatible with RxJS 7 (#41590)
7a86ebf 
Adds a fix to make sure that RxJS v7 Observable is compatible with AsyncPipe. This is a typings-only change.
For more information see: microsoft/TypeScript#43643

PR Close #41590
@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 Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@martinmcwhorter martinmcwhorter martinmcwhorter left review comments

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@benlesh @e-oz @AndrewKushnir @alxhub @petebacondarwin @martinmcwhorter @jessicajaniuk @zarend