grafana / faro-web-sdk

The Grafana Faro Web SDK, part of the Grafana Faro project, is a highly configurable web SDK for real user monitoring (RUM) that instruments browser frontend applications to capture observability signals. Frontend telemetry can then be correlated with backend and infrastructure data for full-stack observability.
https://grafana.com/oss/faro/
Apache License 2.0
690 stars 62 forks source link

Add support for new versions of react-router-dom #582

Closed riboher closed 2 months ago

riboher commented 2 months ago

Description

After updating react-router-dom to its last version v6.23.0, some grafana-react setup fails due to type errors. When using withFaroRouterInstrumentation, types of expected Router don't match.

Types of property 'routes' are incompatible.
    Type 'AgnosticDataRouteObject[]' is not assignable to type 'ReactRouterV6RouteObject[]'.
      Type 'AgnosticDataRouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
        Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
          Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject & { children?: undefined; index: true; }'.
            Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject'.
              Types of property 'action' are incompatible.
                Type 'boolean | ActionFunction<any> | undefined' is not assignable to type '((...args: any[]) => any) | undefined'.
                  Type 'boolean' is not assignable to type '(...args: any[]) => any'.

Proposed solution

Update the necessary dependencies so faro-react works with most recent react-router-dom versions

codecapitano commented 2 months ago

Related issue: #235

codecapitano commented 2 months ago

Hi @riboher the tab got auto closed. Please let us know if you still tun into any problems.

toresbe commented 2 months ago

Hey @codecapitano - I encountered this issue while your release was still going through the pipeline. Talk about responsive support! :)

But unless I'm mistaken, this does not seem to have fixed the issue, I'm still seeing it with 1.7.1:

$ tsc
src/routes.tsx:95:21 - error TS2322: Type '(children: ReactNode, parentPath?: number[] | undefined) => RouteObject[]' is not assignable to type 'ReactRouterV6CreateRoutesFromChildren'.
  Type 'RouteObject[]' is not assignable to type 'ReactRouterV6RouteObject[]'.
    Type 'RouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
      Type 'IndexRouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
        Type 'IndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject & { children?: undefined; index: true; }'.
          Type 'IndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject'.
            Types of property 'action' are incompatible.
              Type 'boolean | ActionFunction<any> | undefined' is not assignable to type '((...args: any[]) => any) | undefined'.
                Type 'boolean' is not assignable to type '(...args: any[]) => any'.

95                     createRoutesFromChildren,
                       ~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/@grafana/faro-react/dist/types/router/v6/types.d.ts:55:5
    55     createRoutesFromChildren: ReactRouterV6CreateRoutesFromChildren;
           ~~~~~~~~~~~~~~~~~~~~~~~~
    The expected type comes from property 'createRoutesFromChildren' which is declared here on type 'ReactRouterV6Dependencies'

Found 1 error in src/routes.tsx:95

Our code:

const routes = (
    <Route errorElement={<SideIkkeFunnet />}>
        (...)
    </Route>
);

export const router = createBrowserRouter(createRoutesFromElements(routes), {basename: basePath});

I've deleted node_modules and package-lock.json and confirmed that the new package-lock.json has only 1.7.1.

Our codebase is full of oddities, so perhaps our routes are set up real odd and I'm offering an edge case here.

codecapitano commented 2 months ago

Hi @riboher thansk a lot for the quick testing. I can confirm the issue. we fixed a problem in regards to data routers which were also having type issues. I'll see that createRoutesFromChildren is causing problems as well.

We'll fix it.

codecapitano commented 2 months ago

One question: Do you use data routers or standard routers?

Asking because if you use data routers you don't need provide createRoutesFromChildrens.

import { matchRoutes } from 'react-router-dom';

import { getWebInstrumentations, initializeFaro, ReactIntegration, ReactRouterVersion } from '@grafana/faro-react';

initializeFaro({
  // ...

  instrumentations: [
    // Load the default Web instrumentations
    ...getWebInstrumentations(),

    new ReactIntegration({
      router: {
        version: ReactRouterVersion.V6_data_router,
        dependencies: {
          matchRoutes,
        },
      },
    }),
  ],
});
codecapitano commented 2 months ago

Would you mind sharing your Faro init code?

codecapitano commented 2 months ago

Pull Request: https://github.com/grafana/faro-web-sdk/pull/585

riboher commented 2 months ago

Hi @codecapitano! I think you tagged me by mistake 😅 . Let me try the new fix and I'll let you know if at least I run into any issues. Thanks for the quick solution!

codecapitano commented 2 months ago

@riboher

Oh that's awesome thank you so much ❤️

I added the link to the PR so that you are aware we are working on it.

riboher commented 2 months ago

Hi @codecapitano, unfortunately the error still shows up 😞 . Let me show you my faro initialization code as well as the use of withFaroRouterInstrumentation

init({
    apiKey,
    app: {
      environment: config.appEnv,
      name: 'app-name',
      version: config.appVersion,
    },
    ignoreErrors: [],
    instrumentations: [
      ...getWebInstrumentations(),
      // Enable this back again once this issue is resolved https://github.com/grafana/faro-web-sdk/issues/260
      // new TracingInstrumentation(),
      new ReactIntegration({
        router: {
          dependencies: {
            matchRoutes,
          },
          version: ReactRouterVersion.V6_data_router,
        },
      }),
    ],
    url,
  });
const routes = getRoutes({ query: { client: queryClient } });
const router = createBrowserRouter(routes, { basename });
const browserRouter = withFaroRouterInstrumentation(router);

const AppRoot = withFaroProfiler(() => {
  return (
    <QueryClientProvider client={queryClient}>
      <ReduxProvider store={store}>
            <RouterProvider router={browserRouter} />
      </ReduxProvider>
    </QueryClientProvider>
  );
});

Same error as before

  Types of property 'routes' are incompatible.
    Type 'AgnosticDataRouteObject[]' is not assignable to type 'ReactRouterV6RouteObject[]'.
      Type 'AgnosticDataRouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
        Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6RouteObject'.
          Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject & { children?: undefined; index: true; }'.
            Type 'AgnosticDataIndexRouteObject' is not assignable to type 'ReactRouterV6BaseRouteObject'.
              Types of property 'action' are incompatible.
                Type 'boolean | ActionFunction<any> | undefined' is not assignable to type '((...args: any[]) => any) | undefined'.
                  Type 'boolean' is not assignable to type '(...args: any[]) => any'.ts(2345)
codecapitano commented 2 months ago

Thanks so much for your help @riboher 🙏

It seems that for some reason it's still pulling the old ReactRouterV6RouteObject[]

With 1.7.1 withFaroRouterInstrumentation uses RouteObjectV6DataRouter[] [1]

Which is referencing the Index/NonIndex routers [2].

Maybe deleting node_modules and doing a fresh install solves the issue?

riboher commented 2 months ago

That definitely fixed the issue @codecapitano! Thanks a lot for the suggestion. What a rookie mistake 😅 . You can close the issue if you want, everything seems fine on my side. Thank you again!

codecapitano commented 2 months ago

Perfect glad it's working now. Then I can release the second fix.

Thanks again for reporting and testing 🙏

toresbe commented 2 months ago

Perfect, this resolved it. Thanks a bunch @codecapitano :)

codecapitano commented 2 months ago

Great @toresbe thanks for confirming 🙏