Skip to content

refactor(core): update FakeNavigation to the latest spec #62017

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Jun 11, 2025

Spec updates are in whatwg/html#10919

For the most part, the updates revolve around the deferred commit handling (with precommitHandler). Updates to redirect allow more options. A committed promise now exists on the transition since commits can be delayed. Tests were made zoneless for easier debugging and timeouts were reduced.

@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 Jun 11, 2025
@pullapprove pullapprove bot requested a review from tbondwilkinson June 11, 2025 20:50
@pullapprove pullapprove bot removed the target: patch This PR is targeted for the next patch release label Jun 11, 2025
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Jun 11, 2025
@ngbot ngbot bot added this to the Backlog milestone Jun 11, 2025
Spec updates are in whatwg/html#10919

For the most part, the updates revolve around the deferred commit
handling (with precommitHandler). Updates to redirect allow more
options. A committed promise now exists on the transition since commits
can be delayed. Tests were made zoneless for easier debugging and
timeouts were reduced.
@atscott atscott force-pushed the fakenavigationupdatesjune11 branch from 45e41dd to 63e7890 Compare June 11, 2025 20:50
@@ -457,6 +458,17 @@ export class FakeNavigation implements Navigation {
return this.disposed;
}

abortTheOngoingNavigation(eventToAbort: InternalFakeNavigateEvent, reason?: Error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: abortOngoingNavigation

I never use the in method names

@@ -457,6 +458,17 @@ export class FakeNavigation implements Navigation {
return this.disposed;
}

abortTheOngoingNavigation(eventToAbort: InternalFakeNavigateEvent, reason?: Error) {
if (!this.navigateEvent || this.navigateEvent !== eventToAbort) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you never have to check whether navigateEvent is truthy, since comparing undefined/null to eventToAbort is always false.

Simplify to if (this.navigateEvent !== eventToAbort)

}
const navigateerrorEvent = new Event('navigateerror', {bubbles: false, cancelable});
const navigateerrorEvent = new Event('navigateerror', {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the ErrorEvent constructor so you can pass error in without having to cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think it exists in node nodejs/node#47587. I was getting an error trying to use it

}
result.finishedReject(reason);
const transition = navigation.transition as InternalNavigationTransition;
Copy link
Contributor

Choose a reason for hiding this comment

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

as InternalNavigationTransition | undefined;

otherwise it looks like your ? is needless

@thePunderWoman
Copy link
Contributor

TESTED=TGP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime requires: TGP This PR requires a passing TGP before merging is allowed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants