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

Support React Router v6 #32

Merged
merged 5 commits into from
Nov 9, 2021
Merged

Support React Router v6 #32

merged 5 commits into from
Nov 9, 2021

Conversation

Grapedge
Copy link
Contributor

@Grapedge Grapedge commented Nov 7, 2021

Support React Router v6: #31

What are the changes?

  • Compatible with Route Object: In React Router v6, Route Object can be used instead of configuration.
  • Intelligent route sorting: Since React Router does not export the sorting algorithm, this is a simple copy and modification of it.
  • Added some interface definitions.
  • Removed matchOptions.

How to migrate

Change Route Config to Route Object:

// before:
const routeConfig = [
  {
    path: '/posts',
    component: Posts,
    breadcrumb: 'Posts',
  },
  {
    path: '/posts/:id',
    component: Post,
    breadcrumb: 'Post',
  },
];

// after:
const routes = [
  {
    path: 'posts',
    element: <Posts />,
    breadcrumb: 'Posts',
    children: [
      {
        path: ':id',
        element: <Post />,
        breadcrumb: 'Post',
      },
    ],
  },
];

Think about how to deal with matchOptions, In React Router v6, we have no concept of strict or exact matching, so you can only configure caseSensitive in the route object:

const routes = [
  {
    path: 'posts',
    element: <Posts />,
    breadcrumb: 'Posts',
    caseSensitive: true,
  },
];

Try to use nested relative paths, otherwise your route-objects may be difficult to maintain:

const routes = [
  {
    path: 'posts',
    element: <PostsLayout />,
    children: [
      {
        index: true,
        element: <Posts />,
        breadcrumb: 'Posts',
      },
      {
        path: ':id',
        element: <Post />,
        breadcrumb: 'Post',
      },
    ],
  },
];

match.url has been replaced by match.pathname, see matchPath, It is also recommended to just use match.params and match.pathname:

const UserBreadcrumb = ({ match }) => {
  const { params } = match;
  return <div>{params.id}</div>;
};

There may be other things that need attention, but I’m sorry I can only think of these for the time being.

Code Review

This change may be relatively large, and some things may require more careful consideration. For formal use, more detailed testing may be required.

Regarding why matchRoutes is not used, I mainly considered the following two points:

  • Although matchRoutes now returns the original route object, But in its function signature, it only returns RouteObject.
  • For cases where routes are not passed in, special handling is required

@icd2k3 icd2k3 self-requested a review November 8, 2021 16:16
@icd2k3
Copy link
Owner

icd2k3 commented Nov 8, 2021

this is fantastic, thanks @Grapedge ! I'll review it today, but on initial glance it looks great

@coveralls
Copy link

coveralls commented Nov 8, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling c61fed0 on Grapedge:react-router-v6 into ee9c360 on icd2k3:master.

const splatPenalty = -2;
const isSplat = (s: string) => s === '*';

function computeScore(path: string, index: boolean | undefined): number {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great that we're now using the same route weight strategy in react-router 6, but my only concern is that we'll have to keep a close eye on if they change that strategy in later versions.

That said, there's not much we can do about it if they're not exporting this scoring method.

Copy link
Owner

@icd2k3 icd2k3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far - could you also bump version in package.json to 3.0.0?

... I know I should have a script do this automatically, but haven't implemented yet - thanks!

@Grapedge
Copy link
Contributor Author

Grapedge commented Nov 9, 2021

but my only concern is that we'll have to keep a close eye on if they change that strategy in later versions

Yes, I read the source code of react router, and I don't think we have a stable way to do this for the time being.

could you also bump version in package.json to 3.0.0?

I have changed the version to 3.0.0 😊

Copy link

@JZFLei JZFLei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@icd2k3 icd2k3 merged commit 53dacc0 into icd2k3:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants