Skip to content

refactor(common): Update FakeNavigation deferred commit to use precom…#60652

Closed
atscott wants to merge 2 commits intoangular:mainfrom
atscott:precommithandler
Closed

refactor(common): Update FakeNavigation deferred commit to use precom…#60652
atscott wants to merge 2 commits intoangular:mainfrom
atscott:precommithandler

Conversation

@atscott
Copy link
Copy Markdown
Contributor

@atscott atscott commented Mar 31, 2025

…mitHandler

This commit updates the FakeNavigation implementation to match the spec's new precommitHandler which replaces the old commit: 'after-transition'.

…mitHandler

This commit updates the FakeNavigation implementation to match the
spec's new `precommitHandler` which replaces the old `commit: 'after-transition'`.
@atscott atscott added target: patch This PR is targeted for the next patch release requires: TGP This PR requires a passing TGP before merging is allowed labels Mar 31, 2025
@angular-robot angular-robot bot added the area: common Issues related to APIs in the @angular/common package label Mar 31, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 31, 2025
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Apr 1, 2025

TGP

@atscott atscott marked this pull request as ready for review April 1, 2025 20:24
@atscott atscott requested a review from tbondwilkinson April 1, 2025 20:24
() => {
const promisesList = handlers.map((handler) => handler());
if (promisesList.length === 0) {
promisesList.push(Promise.resolve());
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.

Promise.all([]) doesn't work for some reason? This is odd

Copy link
Copy Markdown
Contributor Author

@atscott atscott Apr 2, 2025

Choose a reason for hiding this comment

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

This follows the spec word for word. There is a slight difference in timing between Promise.all([]) and Promise.all([Promise.resolve()]).

both of these resolve with '1'

Promise.race([
    Promise.all([]).then(() => '1'),
    Promise.all([Promise.resolve()]).then('2')
])
Promise.race([
    Promise.all([Promise.resolve()]).then('2'),
    Promise.all([]).then(() => '1'),
])
  1. Let promisesList be an empty list.
  2. For each handler of event's navigation handler list:
    1. Append the result of invoking handler with an empty arguments list to promisesList.
  3. If promisesList's size is 0, then set promisesList to « a promise resolved with undefined ».

There is a subtle timing difference between how waiting for all schedules its success and failure steps when given zero promises versus ≥1 promises. For most uses of waiting for all, this does not matter. However, with this API, there are so many events and promise handlers which could fire around the same time that the difference is pretty easily observable: it can cause the event/promise handler sequence to vary. (Some of the events and promises involved include: navigatesuccess / navigateerror, currententrychange, dispose, apiMethodTracker's promises, and the navigation.transition.finished promise.)

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Apr 3, 2025
atscott added a commit that referenced this pull request Apr 3, 2025
…mitHandler (#60652)

This commit updates the FakeNavigation implementation to match the
spec's new `precommitHandler` which replaces the old `commit: 'after-transition'`.

PR Close #60652
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Apr 3, 2025

This PR was merged into the repository by commit 36b9236.

The changes were merged into the following branches: main, 19.2.x

@atscott atscott closed this in 36b9236 Apr 3, 2025
@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 4, 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: common Issues related to APIs in the @angular/common package requires: TGP This PR requires a passing TGP before merging is allowed target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants