GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(core): toSignal accepts Subscribable (in addition to Observable) by divdavem · Pull Request #50162 · angular/angular

Conversation

@divdavem

@divdavem divdavem commented May 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR extends toSignal to accept any Subscribable (instead of only Observable) for consistency with AsyncPipe and for broader compatibility with any observable library (that is compatible with the Subscribable interface).

This is only a type change as the implementation does not use anything else than the Subscribable interface anyway.

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: https://github.com/angular/angular/blob/main/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?

toSignal only accepts Observable.

What is the new behavior?

toSignal accepts any Subscribable.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This PR is similar to PR 39627 that I had opened for AsyncPipe.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ExFlo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@angular-robot angular-robot Bot added the detected: feature PR contains a feature commit label May 5, 2023
JeanMeche
JeanMeche reviewed May 5, 2023
View reviewed changes
Comment thread packages/core/rxjs-interop/src/to_signal.ts Outdated

@JeanMeche JeanMeche May 5, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Observable implements Subscribable, is there a reason you're keeping Observable in the signature ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@divdavem divdavem May 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You're right, but I saw this code in PipeAsync:

  // NOTE(@benlesh): Because Observable has deprecated a few call patterns for `subscribe`,
  // 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>): T|null;

It looks like Typescript has a bug sometimes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JeanMeche, stagefright5, and mephistorine reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@divdavem
divdavem force-pushed the toSignalForSubscribable branch 2 times, most recently from 12d4903 to 253c07a Compare May 5, 2023 14:08
@divdavem divdavem changed the title [WIP] feat: toSignal accepts Subscribable (in addition to Observable) feat: toSignal accepts Subscribable (in addition to Observable) May 5, 2023
@divdavem
divdavem marked this pull request as ready for review May 5, 2023 14:25
@pullapprove
pullapprove Bot requested a review from alxhub May 5, 2023 14:25
@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime cross-cutting: signals labels May 17, 2023
@ngbot ngbot Bot added this to the Backlog milestone May 17, 2023
alxhub
alxhub approved these changes Jun 9, 2023
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pullapprove
pullapprove Bot requested a review from alxhub June 9, 2023 20:49
@alxhub

alxhub commented Jun 9, 2023

Copy link
Copy Markdown
Member

@divdavem thanks for the contribution. Can you change the commit description to label this a fix(core) instead of a feat - this is really fixing a typing issue with toSignal, not introducing new functionality.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@divdavem
divdavem force-pushed the toSignalForSubscribable branch from 253c07a to 209091e Compare June 12, 2023 08:40
@angular-robot angular-robot Bot removed the detected: feature PR contains a feature commit label Jun 12, 2023
@divdavem divdavem changed the title feat: toSignal accepts Subscribable (in addition to Observable) fix(core): toSignal accepts Subscribable (in addition to Observable) Jun 12, 2023
@divdavem

divdavem commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

@alxhub Thank you for reviewing my PR. I have changed the commit description according to your comment.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@divdavem
fix(core): extend toSignal to accept any Subscribable
8b6b190 
Extend toSignal to accept any Subscribable (instead of only Observable)
for consistency with AsyncPipe and for broader compatibility with any
observable library (that is compatible with the Subscribable interface).
This is only a type change as the implementation does not use anything
else than the Subscribable interface anyway.
@divdavem
divdavem force-pushed the toSignalForSubscribable branch from 209091e to 8b6b190 Compare June 12, 2023 08:45
alxhub
alxhub approved these changes Jun 12, 2023
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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,fw-core

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pullapprove
pullapprove Bot requested review from AndrewKushnir, dylhunn and pkozlowski-opensource June 12, 2023 23:02
AndrewKushnir
AndrewKushnir approved these changes Jun 12, 2023
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pkozlowski-opensource pkozlowski-opensource added the target: patch This PR is targeted for the next patch release label Jun 14, 2023
pkozlowski-opensource
pkozlowski-opensource approved these changes Jun 14, 2023
View reviewed changes

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api
Reviewed-for: fw-core

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pkozlowski-opensource
pkozlowski-opensource removed the request for review from dylhunn June 14, 2023 08:43
@pkozlowski-opensource pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Jun 14, 2023
@pkozlowski-opensource

pkozlowski-opensource commented Jun 14, 2023

Copy link
Copy Markdown
Member

This PR was merged into the repository by commit 1ad4d55.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pkozlowski-opensource

pkozlowski-opensource commented Jun 14, 2023

Copy link
Copy Markdown
Member

Thnx @divdavem !!!
divdavem and ExFlo reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pkozlowski-opensource pushed a commit that referenced this pull request Jun 14, 2023
@divdavem @pkozlowski-opensource
fix(core): extend toSignal to accept any Subscribable (#50162)
d9bed48 
Extend toSignal to accept any Subscribable (instead of only Observable)
for consistency with AsyncPipe and for broader compatibility with any
observable library (that is compatible with the Subscribable interface).
This is only a type change as the implementation does not use anything
else than the Subscribable interface anyway.

PR Close #50162
@pkozlowski-opensource pkozlowski-opensource closed this in 1ad4d55 Jun 14, 2023
@divdavem

divdavem commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

@alxhub @AndrewKushnir @pkozlowski-opensource Thank you for reviewing and merging my PR!
AndrewKushnir reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 15, 2023
Calciumdibromid Bot cabr2-bot Epsilon_02
Update angular monorepo (#1927)
77836b8 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/16.0.4/16.1.1) |
| [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/16.0.4/16.1.1) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/16.0.4/16.1.1) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/16.0.4/16.1.1) |
| [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/16.0.4/16.1.1) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/16.0.4/16.1.1) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/16.0.4/16.1.1) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`16.0.4` -> `16.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/16.0.4/16.1.1) |
| [zone.js](https://github.com/angular/angular) ([changelog](https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md)) | dependencies | patch | [`0.13.0` -> `0.13.1`](https://renovatebot.com/diffs/npm/zone.js/0.13.0/0.13.1) |

---

### Release Notes

<details>
<summary>angular/angular (@&#8203;angular/animations)</summary>

### [`v16.1.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1611-2023-06-14)

[Compare Source](angular/angular@16.1.0...16.1.1)

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [71360b3a3e](angular/angular@71360b3) | fix | libraries compiled with v16.1+ breaking with Angular framework v16.0.x ([#&#8203;50715](angular/angular#50715)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [d9bed48eb5](angular/angular@d9bed48) | fix | extend toSignal to accept any Subscribable ([#&#8203;50162](angular/angular#50162)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [5e1d8444ae](angular/angular@5e1d844) | fix | Prevent a component from importing itself. ([#&#8203;50554](angular/angular#50554)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.1.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1610-2023-06-13)

[Compare Source](angular/angular@16.0.6...16.1.0)

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [4e663297c5](angular/angular@4e66329) | fix | error when reading compiled input transforms metadata in JIT mode ([#&#8203;50600](angular/angular#50600)) |
| [721bc72649](angular/angular@721bc72) | fix | resolve deprecation warning with TypeScript 5.1 ([#&#8203;50460](angular/angular#50460)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [68017d4e75](angular/angular@68017d4) | feat | add ability to transform input values ([#&#8203;50420](angular/angular#50420)) |
| [69dadd2502](angular/angular@69dadd2) | feat | support TypeScript 5.1 ([#&#8203;50156](angular/angular#50156)) |
| [c0ebe34cbd](angular/angular@c0ebe34) | fix | add additional component metadata to component ID generation ([#&#8203;50336](angular/angular#50336)) |

##### http

| Commit | Type | Description |
| -- | -- | -- |
| [85c5427582](angular/angular@85c5427) | feat | Introduction of the `fetch` Backend for the `HttpClient` ([#&#8203;50247](angular/angular#50247)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.0.6`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1606-2023-06-13)

[Compare Source](angular/angular@16.0.5...16.0.6)

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [05ac0868c9](angular/angular@05ac086) | fix | avoid duplicated content during hydration while processing a component with i18n ([#&#8203;50644](angular/angular#50644)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v16.0.5`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1605-2023-06-08)

[Compare Source](angular/angular@16.0.4...16.0.5)

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [703b8fcac1](angular/angular@703b8fc) | fix | do not remove comments in component styles ([#&#8203;50346](angular/angular#50346)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [2b6da93e19](angular/angular@2b6da93) | fix | incorrectly throwing error for self-referencing component ([#&#8203;50559](angular/angular#50559)) |
| [c992109d6c](angular/angular@c992109) | fix | wait for HTTP in `ngOnInit` correctly before server render ([#&#8203;50573](angular/angular#50573)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [c0d4086c6e](angular/angular@c0d4086) | fix | surface errors during rendering ([#&#8203;50587](angular/angular#50587)) |

<!-- CHANGELOG SPLIT MARKER -->

</details>

<details>
<summary>angular/angular (zone.js)</summary>

### [`v0.13.1`](https://github.com/angular/angular/blob/HEAD/packages/zone.js/CHANGELOG.md#v0131-httpsgithubcomangularangularcomparezonejs-0130zonejs-v0131-2023-06-09)

[Compare Source](angular/angular@zone.js-0.13.0...zone.js-0.13.1)

##### Bug Fixes

-   **zone.js:** enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50467](angular/angular#50467)) ([381cb98](angular/angular@381cb98))
-   **zone.js:** enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50530](angular/angular#50530)) ([7837f71](angular/angular@7837f71))
-   **zone.js:** patch entire promise in node ([#&#8203;50552](angular/angular#50552)) ([cb31dbc](angular/angular@cb31dbc)), closes [#&#8203;50513](angular/angular#50513) [#&#8203;50457](angular/angular#50457) [#&#8203;50414](angular/angular#50414) [#&#8203;49930](angular/angular#49930)
-   **zone.js:** revert Mocha it.skip, describe.skip method patch ([#&#8203;49329](angular/angular#49329)) ([5a2b622](angular/angular@5a2b622))

##### Features

-   **zone.js:** jest 29 should ignore uncaught error console log ([#&#8203;49325](angular/angular#49325)) ([bc412fd](angular/angular@bc412fd)), closes [#&#8203;49110](angular/angular#49110)

##### Reverts

-   Revert "fix(zone.js): enable monkey patching of the `queueMicrotask()` API in node.js ([#&#8203;50467](angular/angular#50467))" ([#&#8203;50529](angular/angular#50529)) ([7567348](angular/angular@7567348)), closes [#&#8203;50467](angular/angular#50467) [#&#8203;50529](angular/angular#50529) [#&#8203;50529](angular/angular#50529)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTUuMiIsInVwZGF0ZWRJblZlciI6IjM1LjExNy4yIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1927
Reviewed-by: Epsilon_02 <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
@AndrewKushnir

AndrewKushnir commented Jun 17, 2023

Copy link
Copy Markdown
Contributor

@divdavem thanks for the contribution 👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jul 18, 2023

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 18, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@divdavem @ChellappanRajan
fix(core): extend toSignal to accept any Subscribable (angular#50162)
ed28744 
Extend toSignal to accept any Subscribable (instead of only Observable)
for consistency with AsyncPipe and for broader compatibility with any
observable library (that is compatible with the Subscribable interface).
This is only a type change as the implementation does not use anything
else than the Subscribable interface anyway.

PR Close angular#50162
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeanMeche JeanMeche JeanMeche left review comments
@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes
@alxhub alxhub alxhub approved these changes
@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cross-cutting: signals 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@divdavem @alxhub @pkozlowski-opensource @AndrewKushnir @JeanMeche
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.