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

[Platform] Add exact property to register route in plugin #69110

Closed
XavierM opened this issue Jun 13, 2020 · 9 comments · Fixed by #69772
Closed

[Platform] Add exact property to register route in plugin #69110

XavierM opened this issue Jun 13, 2020 · 9 comments · Fixed by #69772
Assignees
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0

Comments

@XavierM
Copy link
Contributor

XavierM commented Jun 13, 2020

In the security solution, we are having different routes like

  • app/security/overview
  • app/security/alerts
  • app/security/hosts
  • etc...

we would like to redirect the route app/security to app/security/overview by doing that

  core.application.register({
      id: 'security',
      title: 'Security',
      appRoute: 'app/security',
      navLinkStatus: AppNavLinkStatus.hidden,
      mount: async (params: AppMountParameters) => {
        const [{ application }] = await core.getStartServices();
        application.navigateToApp(`${APP_ID}:${SecurityPageName.overview}`, { replace: true });
        return () => true;
      },
    });

However, when we are doing that, we are breaking all the different routes in our app. Since everything is going to the mount of the route app/security. That's why we think to add an exact property will resolve our problem.

@XavierM XavierM added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 labels Jun 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

In the security solution, we are having different routes like

Just to be sure to understand:

All yours 'routes' are mounted as distinct apps, right? Else I guess we wouldn't encounter such an issue?

Follow-up question (if that's the case): This decision was driven by the need to have multiple navlinks to the same 'app', is that it?

@joshdover
Copy link
Contributor

Follow-up question (if that's the case): This decision was driven by the need to have multiple navlinks to the same 'app', is that it?

I believe so, yes. But I also know that Enterprise Search would like have a similar routing scheme for their two different apps. I think it's approximately:

  • /app/enterprise_search/workplace_search -> workplace app
  • /app/enterprise_search/app_search -> app search app
  • /app/enterprise_search -> a common landing page

@constancecchen is that about right?

@cee-chen
Copy link
Member

Yes, that's right - we'd love to have our routing set up that way! 😄

@pgayvallet
Copy link
Contributor

I have no real objections to add an option that would reflect the <Route> exact property.

The only thing I can think of is that navigateToApp may behave quite strangely in some edge cases:

core.application.register({
      id: 'security',
      title: 'Security',
      appRoute: 'app/security',
      exactRoute: true,
      mount: async (params: AppMountParameters) => {
        .....
      },
});

// later

// will redirect to `/app/security/some/path` which is not handled by the route, resulting on a 404
core.application.navigateToApp('security', {path: 'some/path'});

Not sure this is something we should really be concerned about? We could add a check in navigateToApp to check is the app's route is exact and raise an error when using path option with an exact route. Not sure this is really necessary though.

@XavierM
Copy link
Contributor Author

XavierM commented Jun 16, 2020

or maybe we can have something like that

core.application.register({
      id: 'security',
      title: 'Security',
      appRoute: 'app/security',
      redirectTo: {appId} or {appRoute},
});

@pgayvallet
Copy link
Contributor

The issue with the path parameter remains the same (what should core.application.navigateToApp('security', {path: 'some/path'}); do).

I also think that the route behavior should remains strictly the registrant responsibility, so I think i'd prefer the exact/exactRoute option.

@pgayvallet pgayvallet self-assigned this Jun 23, 2020
@pgayvallet
Copy link
Contributor

After sync discussion with the team:

  • We will be implementing the exactRoute option for 7.9
  • Depending on the need from other apps/teams, we will see post 8.0 if we implement an equivalent of legacy's NavLink to allow registering multiple navLinks pointing to different paths of the same app. If we do so, we would then probably deprecate the exactRoute option in favor of using this new feature.

@cee-chen
Copy link
Member

cee-chen commented Sep 1, 2020

Just wanted to give an update on Enterprise Search's end - we eventually decided on registering our overview app as /app/enterprise_search/overview instead of /app/enterprise_search with exactRoute: true after all.

We made this choice because we ended up realizing exactRoute meant we couldn't ever set any subrouting within the overview plugin, such as /app/enterprise_search/upgrade (for example). We totally understand that it's a limitation of React Router itself of course, but we wanted to leave our overview plugin open for potential routing expansion in the future instead of locking ourselves into /app/enterprise_search only.

Apologies that we ended up not using this super useful functionality that you implemented, but thanks so much for giving us the option / thinking of us!! 🙇‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants