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

Handle Angular navigation errors #24517

Merged

Conversation

atomfrede
Copy link
Member

This PR uses https://angular.io/api/router/withNavigationErrorHandler to handle navigation error events, which can't be handled by our http interceptor error handlers.

I tried to extract a dedicated class, but failed miserable to have that injected in the config.ts file. Either I got no injector defined or the injected class was wrong/not constructed such that it failed with ... has not method .... Looks like I need to refresh my angular skills :(

updates #24396


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@atomfrede atomfrede requested a review from mraible December 10, 2023 13:17
@DanielFran DanielFran changed the title handle agnular navigation errors Handle Angular navigation errors Dec 10, 2023
withNavigationErrorHandler((e: NavigationError) => {
const router = inject(Router);
if (e.error.status === 403) {
router.navigate(['/accessdenied'])
Copy link
Contributor

@mraible mraible Dec 10, 2023

Choose a reason for hiding this comment

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

IntelliJ doesn't like that there's no await or .then() here, but it works, so I'm OK with not having it.

Screenshot 2023-12-10 at 9 35 30 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

It also says the httpInterceptorProviders import can be shorted to:

import { httpInterceptorProviders } from './core/interceptor';

@mraible
Copy link
Contributor

mraible commented Dec 10, 2023

CI tests seem to be failing because Router isn't imported. After making this PR work locally, the router imports should be:

import {
  RouterFeatures,
  TitleStrategy,
  provideRouter,
  withComponentInputBinding,
  withDebugTracing,
  withNavigationErrorHandler, NavigationError, Router
} from '@angular/router';

mraible
mraible previously approved these changes Dec 10, 2023
Copy link
Contributor

@mraible mraible left a comment

Choose a reason for hiding this comment

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

I tested these changes locally, and everything works as expected.

Screenshot 2023-12-10 at 9 38 24 AM

@atomfrede atomfrede force-pushed the 24396-handle-ng-navigation-errors branch 2 times, most recently from dccf42d to ef9d2a4 Compare December 10, 2023 16:49
@atomfrede
Copy link
Member Author

CI tests seem to be failing because Router isn't imported. After making this PR work locally, the router imports should be:

import {
  RouterFeatures,
  TitleStrategy,
  provideRouter,
  withComponentInputBinding,
  withDebugTracing,
  withNavigationErrorHandler, NavigationError, Router
} from '@angular/router';

Totally forgot the imports, sorry about that

@atomfrede atomfrede force-pushed the 24396-handle-ng-navigation-errors branch from ef9d2a4 to ec43ca5 Compare December 10, 2023 16:55
@mraible mraible enabled auto-merge (squash) December 10, 2023 22:55
@mraible
Copy link
Contributor

mraible commented Dec 11, 2023

Reverted actions/labeler in #24528 because of this failure in this PR.

@mraible
Copy link
Contributor

mraible commented Dec 11, 2023

It'd be great to merge this before 8.1, but I don't think we should force anything if CI says no.

@atomfrede
Copy link
Member Author

For the failing sample tests, it fails during startup of all dependency containers, it looks like a timeout.

@atomfrede atomfrede force-pushed the 24396-handle-ng-navigation-errors branch from 0d364b2 to 5018b70 Compare December 11, 2023 20:09
@mshima
Copy link
Member

mshima commented Dec 11, 2023

CI error is not related to this PR.
Reported in #24533

@atomfrede atomfrede force-pushed the 24396-handle-ng-navigation-errors branch from 5018b70 to 489ec73 Compare December 12, 2023 05:21
@mshima mshima closed this Dec 12, 2023
auto-merge was automatically disabled December 12, 2023 16:02

Pull request was closed

@mshima mshima reopened this Dec 12, 2023
@DanielFran DanielFran enabled auto-merge December 12, 2023 16:21
@DanielFran DanielFran merged commit 5cefdd0 into jhipster:main Dec 12, 2023
78 of 88 checks passed
@atomfrede
Copy link
Member Author

@DanielFran
Copy link
Member

@atomfrede approved

@deepu105 deepu105 added this to the 8.2.0 milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants