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

"page not found" when the matching route is not under the first matching parent #307

Closed
vlukashov opened this issue Nov 9, 2018 · 3 comments · Fixed by #438
Closed

"page not found" when the matching route is not under the first matching parent #307

vlukashov opened this issue Nov 9, 2018 · 3 comments · Fixed by #438
Assignees
Labels
bug Something isn't working hilla

Comments

@vlukashov
Copy link

With the routes config below Vaadin Router should render <x-layout-b><x-b></x-b></x-layout-b> for the /b pathname. However, currently it throws Error: [Vaadin.Router] Page not found (/b).

// describe('parent layouts rendering', () => {
it('should render the matching child route even if it is not under the first matching parent', async() => {
  router.setRoutes([
    {
      path: '/',
      component: 'x-layout-a',
      children: [
        { path: '/a', component: 'x-a' },
      ]
    },
    {
      path: '/',
      component: 'x-layout-b',
      children: [
        { path: '/b', component: 'x-b', }
      ]
    },
  ]);

  await router.render('/b');

  verifyActiveRoutes(router, ['/', '/b']);
  checkOutlet(['x-layout-b', 'x-b']);
});
@vlukashov vlukashov added the bug Something isn't working label Nov 9, 2018
vlukashov pushed a commit that referenced this issue Nov 9, 2018
@vlukashov
Copy link
Author

Workaround: list all child routes from all matching parents under one parent and use a custom route action to determine which parent component to render based on a given context:

router.setRoutes([
  {
    path: '/',
    action: (context, commands) => {
      return shouldRenderLayoutBForTheContext(context)
        ? commands.component('x-layout-b')
        : commands.component('x-layout-a');
    },
    children: [
      { path: '/a', component: 'x-a' },
      { path: '/b', component: 'x-b' },
    ]
  }
]);

@ZempTime
Copy link

ZempTime commented Nov 21, 2018

The workaround for this also prevents using named parameters in the children routes of "/". I've been working on ripping out our existing routing and plugging this into our app in a rather large and invasive PR, and wasn't having any troubles, but then ran into the lack of parameterized routes. This is a priority issue for our adoption or not of @vaadin/router over the longer term. I was wondering:

  • Is there a plan/eta on this getting fixed? -> will wait and work around for time being
  • Are you open to pull requests? -> this has something to do with resolve & matchRoute not traversing down the second branch of the test case (I'm not sure exactly how this next & context work together)
  • No plan/no estimate -> we'll have to go find another router

Also, when I swapped:

        if (match) {
          return {
            done: false,
            value: {
              route,
              keys: match.keys,
              params: match.params,
              path: match.path
            }
          };
        }

and made

      if (match && children) {
        while (childIndex < children.length) {
          if (!childMatches) {
            const childRoute = children[childIndex];
            childRoute.parent = route;

            let matchedLength = match.path.length;
            if (matchedLength > 0 && pathname.charAt(matchedLength) === '/') {
              matchedLength += 1;
            }

            childMatches = matchRoute(childRoute, pathname.substr(matchedLength), ignoreLeadingSlash, match.keys, match.params);
          }

          const childMatch = childMatches.next(routeToSkip);
          if (!childMatch.done) {
            return {
              done: false,
              value: childMatch.value
            };
          }

          childMatches = null;
          childIndex++;
        }
      }

execute first, it matched against /b but was missing the / match of the parent route

@ZempTime
Copy link

Right here it will match correctly against /b child route, then parent will override matches.value.route and set it back to an /a-related route object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hilla
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants