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

[APM] Improve client-side routes #51963

Closed
cauemarcondes opened this issue Dec 2, 2019 · 8 comments · Fixed by #104274
Closed

[APM] Improve client-side routes #51963

cauemarcondes opened this issue Dec 2, 2019 · 8 comments · Fixed by #104274
Assignees
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan Team:APM - DEPRECATED Use Team:obs-ux-infra_services. technical debt Improvement of the software architecture and operational architecture

Comments

@cauemarcondes
Copy link
Contributor

cauemarcondes commented Dec 2, 2019

The client side routes should resemble the server side routes:

  • No globally shared declaration of path and query params (aka let's kill useUrlParams and resolveUrlParams)
  • statically typed boundaries for path and query params (use io-ts)
  • Instead of calling useLocation or useUrlParams components should receive their route params as props.

Currently client side routes look like:

  {
    exact: true,
    path: '/settings/agent-configuration',
    component: () => (
      <Settings>
        <AgentConfigurations />
      </Settings>
    ),
    breadcrumb: i18n.translate(
      'xpack.apm.breadcrumb.settings.agentConfigurationTitle',
      {
        defaultMessage: 'Agent Configuration'
      }
    ),
    name: RouteName.AGENT_CONFIGURATION
  },

In the future it should look something like

"/services/{serviceName}/errors": {
  params: {
    path: t.type({
      serviceName: t.string
    }),
    query: t.partial({
      rangeFrom: t.string,
      rangeTo: t.string
    })
  },
  handler: async ({ query, path }) => {
    return <ErrorGroupDetails routeParams={{ query, path }} />;
  }
}

Then from the component:

function ErrorGroupDetails({ routeParams: RouteParams<'/services/{serviceName}/errors'> }) {
  routeParams.path.serviceName // required string
  routeParams.query.rangefrom // optional string
}

Retrieve route params from deeply nested component

We could also make a helper to make it easier to obtain route params deep down in the tree (although we can't ensure it's called from the right component, so its use should be limited):

const { path, query } = getRouteParams('/services/:serviceName/nodes')

Linking to a route

Get the full href string:

const href = link('/services/{service}/transaction', {
  path: { service: 'opbeans-node' },
  query: { foo: 'bar'}
}).href();

push new item to history API:

link('/services/{service}/transaction', {
  path: { service: 'opbeans-node' }
  query: { foo: 'bar'}
}).push();

Replace existing item on history API

link('/services/{service}/transaction', {
  path: { service: 'opbeans-node' }, 
  query: { foo: 'bar'}
}).replace();

As a component:

<APMLink 
  path="/services/{service}/transaction" 
  params={{ 
    path: { service: 'opbeans-node' }, 
    query: { foo: 'bar'}
  }} 
/>
@cauemarcondes cauemarcondes added [zube]: Inbox Team:APM - DEPRECATED Use Team:obs-ux-infra_services. technical debt Improvement of the software architecture and operational architecture labels Dec 2, 2019
@cauemarcondes cauemarcondes self-assigned this Dec 2, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@sorenlouv
Copy link
Member

sorenlouv commented Mar 6, 2020

I have intentionally left out breadcrumb support in the issue above. It's currently making everything even more complicated so we might drop our implementation and try something a lot simpler (more dumb).

@dgieselaar
Copy link
Member

react-router v6 has first-class support for nested routes as config.

Here's a POC for inferring params from routes:

image

@sorenlouv
Copy link
Member

Super cool @dgieselaar. Do we need to wait for react-router v6 to land before starting this?

@dgieselaar
Copy link
Member

@sqren no, we can work with react-router-config in the meantime: #104274

@dgieselaar dgieselaar removed their assignment Aug 25, 2021
@ogupte ogupte self-assigned this Aug 30, 2021
@ogupte ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan Team:APM - DEPRECATED Use Team:obs-ux-infra_services. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants