Skip to content

refactor(core): support an opt-in sync version of toObservable#60640

Closed
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:toobservable-sync-emit
Closed

refactor(core): support an opt-in sync version of toObservable#60640
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:toobservable-sync-emit

Conversation

@alxhub
Copy link
Copy Markdown
Member

@alxhub alxhub commented Mar 31, 2025

This commit adds a flag forceSyncFirstEmit which opts in to the pending new behavior for toObservable, which emits the first value synchronously. This flag is only really meant for use during a short migration period while we update g3, and is not meant for prolonged usage. As a result, it's marked deprecated.

@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Mar 31, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 31, 2025
@alxhub alxhub added the target: major This PR is targeted for the next major release label Apr 1, 2025
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

This commit adds a flag `forceSyncFirstEmit` which opts in to the pending
new behavior for `toObservable`, which emits the first value synchronously.
This flag is only really meant for use during a short migration period
while we update g3, and is not meant for prolonged usage. As a result, it's
marked deprecated.
@alxhub alxhub force-pushed the toobservable-sync-emit branch from 67730b3 to 8d94afc Compare April 1, 2025 15:02
@pullapprove pullapprove bot requested review from crisbeto, kirjs and mmalerba April 1, 2025 15:02
Copy link
Copy Markdown
Contributor

@kirjs kirjs 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit/optional.
I'd just have two functions cleanUp and cleanUpFrominjector, or something similar it'd have some duplicate code, but would be much nicer to read

expect(emits()).toBe(1);
});

describe('synchronous emit', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a bunch of logic around clean up, would be nice to have that tested.

@pullapprove pullapprove bot requested a review from atscott April 1, 2025 15:04
Copy link
Copy Markdown
Contributor

@mmalerba mmalerba 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

@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Apr 1, 2025
@thePunderWoman
Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit 57a240b.

The changes were merged into the following branches: main

alxhub added a commit to alxhub/angular that referenced this pull request Apr 2, 2025
…le` (angular#60640)"

This reverts commit 57a240b. We're no
longer attempting to convert `toObservable` to a synchronous API.
atscott pushed a commit that referenced this pull request Apr 2, 2025
…le` (#60640)" (#60449)

This reverts commit 57a240b. We're no
longer attempting to convert `toObservable` to a synchronous API.

PR Close #60449
@angular-automatic-lock-bot
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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants