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

Adding ability to use a 404 route #98

Closed
wants to merge 1 commit into from
Closed

Conversation

chrisgubbels
Copy link
Contributor

In page.js you can define a 404 route, using '*' as your path.

In the router, this route's callback is still called after the callback of any route that might also have a triggered routePath.

What we want is for the * path to only be activated if the context has not been handled.

To test:

  1. Tests should pass.
  2. Checkout this branch in digital-tools, yarn link web-component-router and run yarn serve in the example-dashboard-app
    2a. Go to the dashboard page, the 404 should not show. Add /sdfahsdkjfh or something to the url, the 404 page should show.

Copy link
Contributor

@barronhagerman barronhagerman left a comment

Choose a reason for hiding this comment

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

I agree these changes would be necessary for a "*" fallback route, in order to avoid always showing that route. However, I think it could only be used if there are no other applications in the same domain.

For example, consider an application on https://localhost/app1 with a "*" route to show a 404 error page. If you click on a link to https://localhost/app2 in the app1 app, I think the 404 page would be displayed rather than redirecting to the expected location because the 404 route handler would set context.handled = true, and this line in the unhandled handler would never be executed:

window.location.href = url.toString();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants