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

feat: pluggable matcher #2166

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Mar 7, 2024

Background

Original issue: #2132. In short, the performance of adding a large number of routes is slow, and is particularly problematic with SSR.

This PR explores the idea of a 'pluggable matcher'. This allows the matcher to be passed into the router, rather than being created internally. The potential benefits are twofold:

  1. In an SSR scenario, the matcher can be created upfront, rather than processing the individual routes for each request.
  2. Custom matchers can be used, potentially supporting different features or faster implementations.

The code presented here is a long way from being merged, but hopefully it'll work as a starting point to discuss how to proceed.

SSR usage

To get the benefits with SSR, we'd need to replace this:

const router = createRouter({
  history,
  routes: [/* ... */],
  strict,
  sensitive
})

with this:

// Do this once
const matcher = createDefaultMatcher({
  routes: [/* ... */],
  strict,
  sensitive
})

// Do this for each request
const router = createRouterWithMatcher({
  history,
  matcher: matcher.clone()
})

The clone() call creates a copy of the matcher, allowing for more routes to be added or removed without impacting the original matcher.

The performance of this is really good. Calling clone() is very fast and we skip all the overhead of processing the individual routes.

Custom matchers

This is trickier.

The usage is similar to the previous example, but with a custom implementation of matcher.

TypeScript isn't my strong suit, so perhaps this isn't as difficult as it seemed, but I found trying to get the types working as I wanted really difficult. The code in this PR is much less ambitious than some of my other attempts, but hopefully it'll still illustrate the difficulties.

For example, I tried writing a custom matcher that only supported the options path, name, component and components. You can see that at:

If you search through the code for Pain point, you'll find various points where I had to do things that didn't really make sense in the context of that matcher implementation. Some of those may be necessary for other reasons, but in many cases I was adding things just to appease TS. For example, the score and re properties are completely redundant, but the types force them to be included.

I did experiment with changing those types, adding generics all over the place, but it got a bit out of hand and I decided to aim a little lower for this initial PR. Instead, I chose to focus just on the route config.

As SimpleRouterMatcher only supports 4 options for the route, I wanted the types to reflect that. So SimpleRoute is a type that defines that format, with the matcher having type:

interface SimpleRouterMatcher extends GenericRouterMatcher<SimpleRoute> {}

You'll find RC used a generic in various places in the code. RC stands for 'route config'.

This then has an impact on the type of the addRoute() method of the router itself. New routes need to be added using the SimpleRoute format. e.g.:

const matcher = createSimpleRouterMatcher({
  routes: [
    {
      path: '/',
      component: HomeView
    }
  ]
})

const router = createRouterWithMatcher({ history, matcher })

// The type here is 'SimpleRoute', so options like 'strict' aren't available
router.addRoute({
  path: '/about',
  component: AboutView
})

This almost works. The major problem is $router and useRouter(), neither of which take account of the generic.

Other problems

The options property of the Router type is a bit fiddly. It needs to take account of the options passed to createRouterWithMatcher(), which aren't the same as createRouter().

Next steps

If we just want the performance benefits for SSR usage, we don't need support for custom matchers. That would dodge most of the type problems.

The custom matcher feature may tie into #2148. That PR is currently implemented as changes to the default matcher, but it could instead be implemented as a separate matcher implementation.

If the type problems can be overcome, I think we'd also need to consider adding helpers to make custom matchers easier to write. I found myself duplicating a lot of logic from the default matcher when I was writing SimpleRouterMatcher. If we do choose to pursue this further, I recommend anyone wanting to give feedback to attempt writing a custom matcher. I found going through that process quite enlightening.

Depending on what direction we choose to go in, we should also consider an RFC. At the very least we'd need some feedback from the SSR meta-frameworks about whether this is workable for them.

Relationship to other PRs

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit be657db
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/65e91f30f04dbf000835fa16

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.93%. Comparing base (089378b) to head (be657db).
Report is 3 commits behind head on main.

Files Patch % Lines
packages/router/src/matcher/index.ts 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2166      +/-   ##
==========================================
- Coverage   91.01%   90.93%   -0.09%     
==========================================
  Files          24       24              
  Lines        1135     1136       +1     
  Branches      351      351              
==========================================
  Hits         1033     1033              
- Misses         63       64       +1     
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

2 participants