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

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

Closed
riboher opened this issue May 6, 2024 · 15 comments · Fixed by #584
Closed

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

riboher opened this issue May 6, 2024 · 15 comments · Fixed by #584
Assignees
Labels
bug Report a bug

Comments

@riboher
Copy link

riboher commented May 6, 2024

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

@riboher riboher added the feature Request a new feature label May 6, 2024
@codecapitano codecapitano self-assigned this May 6, 2024
@codecapitano codecapitano added bug Report a bug and removed feature Request a new feature labels May 6, 2024
@codecapitano
Copy link
Collaborator

Related issue: #235

@codecapitano codecapitano linked a pull request May 6, 2024 that will close this issue
3 tasks
@codecapitano
Copy link
Collaborator

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

@toresbe
Copy link

toresbe commented May 6, 2024

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 codecapitano reopened this May 6, 2024
@codecapitano
Copy link
Collaborator

codecapitano commented May 6, 2024

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
Copy link
Collaborator

codecapitano commented May 6, 2024

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
Copy link
Collaborator

Would you mind sharing your Faro init code?

@codecapitano
Copy link
Collaborator

Pull Request: #585

@riboher
Copy link
Author

riboher commented May 6, 2024

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
Copy link
Collaborator

@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
Copy link
Author

riboher commented May 6, 2024

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
Copy link
Collaborator

codecapitano commented May 6, 2024

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
Copy link
Author

riboher commented May 6, 2024

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
Copy link
Collaborator

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

Thanks again for reporting and testing 🙏

@toresbe
Copy link

toresbe commented May 6, 2024

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

@codecapitano
Copy link
Collaborator

Great @toresbe thanks for confirming 🙏

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

Successfully merging a pull request may close this issue.

3 participants