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

Add zoneless scheduler to the ApplicationRef.isStable indicator #53579

Closed

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Dec 14, 2023

Commit 1:
test(core): Add scheduler in tests to tie into ApplicationRef.isStable

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

Commit 2
refactor(core): Move change detection scheduler implementation to core

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

@atscott atscott added state: blocked area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Dec 14, 2023
@ngbot ngbot bot added this to the Backlog milestone Dec 14, 2023
@atscott atscott force-pushed the schedulerStablePendingRenderTasks branch 4 times, most recently from fe5326e to a7c4469 Compare December 20, 2023 16:10
@atscott atscott force-pushed the schedulerStablePendingRenderTasks branch from a7c4469 to df515b4 Compare December 20, 2023 16:11
@atscott atscott marked this pull request as ready for review January 5, 2024 16:51
@atscott atscott force-pushed the schedulerStablePendingRenderTasks branch from df515b4 to a01dd83 Compare January 5, 2024 17:35
@atscott atscott requested a review from alxhub January 9, 2024 22:06
@atscott atscott force-pushed the schedulerStablePendingRenderTasks branch from a01dd83 to 16ec80c Compare January 9, 2024 22:12
@eneajaho
Copy link
Contributor

eneajaho commented Jan 9, 2024

Hi
Regarding naming, I was thinking of having something like we do in http client (provideHttpClient(withFetch())).

provideChangeDetection(withZone());

The only issue with that is that a required migration would be needed to migrate every app to use it.

And then have it removed for default zoneless change detection.

@JeanMeche
Copy link
Member

Hi Regarding naming, I was thinking of having something like we do in http client (provideHttpClient(withFetch())).

provideChangeDetection(withZone());

The only issue with that is that a required migration would be needed to migrate every app to use it.

And then have it removed for default zoneless change detection.

Then probably the default could be zonefull, and the api could be provideChangeDetection(withoutZone());

@eneajaho
Copy link
Contributor

eneajaho commented Jan 9, 2024

Hi Regarding naming, I was thinking of having something like we do in http client (provideHttpClient(withFetch())).

provideChangeDetection(withZone());

The only issue with that is that a required migration would be needed to migrate every app to use it.
And then have it removed for default zoneless change detection.

Then probably the default could be zonefull, and the api could be provideChangeDetection(withoutZone());

Yeah that's the point, because the default would be without zone, it wouldn't be necessary to write it at all.

@atscott
Copy link
Contributor Author

atscott commented Jan 9, 2024

We already have the provideZoneChangeDetection provider function. When zoneless becomes the default, we can deprecate this function and have a migration to remove it entirely. Naming of this function is still an open question but this does not add it to the public API yet so we aren't really at the bikeshedding stage

edit - Excerpt from the design doc:

We have an existing API that is provideZoneChangeDetection that allows developers to configure how ZoneJS behaves and drives change detection scheduling [...] A tempting name might be something like provideZonelessChangeDetection as the other option. With zoneless being our ideal future, it would be nice to avoid the “Zone” name in the API, for example provideChangeDetectionScheduler(). On the flipside, if zoneless is the future, then ideally we deprecate this provider and have it the new default instead of provideZoneChangeDetection, which is currently built into both TestBed and application bootstrap. In that respect, provideZonelessChangeDetection might be a good name while the default is zones.

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.
This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.
@atscott atscott force-pushed the schedulerStablePendingRenderTasks branch from 16ec80c to a354f03 Compare January 9, 2024 23:32
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Jan 9, 2024
atscott added a commit that referenced this pull request Jan 10, 2024
…le` (#53579)

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

PR Close #53579
atscott added a commit that referenced this pull request Jan 10, 2024
#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close #53579
@atscott
Copy link
Contributor Author

atscott commented Jan 10, 2024

This PR was merged into the repository by commit 5978b3d.

@atscott atscott closed this in 60dfcc2 Jan 10, 2024
atscott added a commit that referenced this pull request Jan 10, 2024
#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close #53579
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…le` (angular#53579)

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

PR Close angular#53579
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
angular#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close angular#53579
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
…le` (angular#53579)

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

PR Close angular#53579
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
angular#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close angular#53579
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
…le` (angular#53579)

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

PR Close angular#53579
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
angular#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close angular#53579
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
…le` (angular#53579)

This commit updates the test scheduler implementation to contribute to
ApplicationRef stableness.

PR Close angular#53579
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
angular#53579)

This commit moves the implementation of the change detection scheduler
used for testing to angular/core along with a (private export) provider function.

Note: Naming of the provider function is absolutely not final (and not
public API). I would prefer one that did not mention "zones"
but the easiest thing for now is to have a "Zone" and "Zoneless" naming
scheme.

PR Close angular#53579
@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 Feb 10, 2024
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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants