Routing overridability

[gryphonmyers]

This PR seeks to do the following

• Address Deep Integration with Vue Router #9
• Refactor some of the routing assumptions in the plugin to allow for overridability. This opens the door to swapping out the underlying matcher (e.g. from pathToRegexp to Vue Router)
• Pass routing information through to the client, allowing for client-side routing integration of application routes.

At present, note that vue-router has been added to the dependencies and the matchRoutes implementation in route.shared.ts has been switched from the pathToRegexp matcher to a Vue Router matcher. This was to verify as a POC, but probably ideally the matcher should be customizable via a plugin interface. At the very least, configuration allowing the user to choose the underlying matcher should be exposed.

Also note that previously, functional routes were expected to return objects in a { matchValue: boolean|number, routeParams: Record<string, unknown> } signature. I have expanded this to allow for returning a routing string instead, which will then be parsed and matched as any other path string. The reason for this is that functional routes are compiled on the server, and once the routes get to the client, we should have them in a format that is easily understood by the configured matcher. In the case of Vue Router, this is a Vue Router path string (similar to pathToRegexp, but slightly different) so it will not know what to do with { matchValue: boolean|number, routeParams: Record<string, unknown> }. For the same reason, I have added a warning when users try to use that signature with the Vue Router matcher.

Please review and let me know of any concerns. I do have a basic proof of concept working end-to-end, and can share the specifics of the front-end integration if desired (I'm planning to eventually package them into an additional plugin or at the very least documentation).

[gryphonmyers]

I see the following options for how we expose configuration of the matcher:

• Plugin interface exposed when instantiating the plugin (e.g. pass ssr({matchRoutes: vueRouterMatchRoutes}) or ssr({ routeMatchers: { vueRouter, someOtherRouterIntegration } }) into vite config).
• Additional supported matchers are built into the source and configurable via enum (e.g. ssr({ routeMatcher: 'vue-router' }) with no additional imports needed).
• This is sort of in addition to either of the above approaches, but perhaps users could select from matchers in specific *.route.js files (e.g. export { route: '/:myParam?', matcher: 'vue-router' }). This should allow for using multiple matchers within the same application, which would preserve the open-endedness that exists in other parts of the plugin.

[brillout]

Looking forward to have a look at all of this!

I do have a basic proof of concept working end-to-end

Can I see this somewhere? I'd love to see the end result of your work.

[gryphonmyers]

Here let me do this - I'll get this setup working in the vue-router example and make those changes part of this PR.

[gryphonmyers]

@brillout ^

[brillout]

Here let me do this - I'll get this setup working in the vue-router example and make those changes part of this PR.

Perfect.

I will finish my context props work and then have a look at your PR. Very curious to see the deep integration.

[brillout]

Couple of notes:

• I agree with some of your refactoring but let's focus this PR on vue integration. Including refactoring in a PR: makes it harder for me to comprehend what the PR changes, and makes it harder to resolve merge conflicts (which will actually occur after I merge my latest to changes to master).
• We can create a seperate refactor PR before or after this PR, whatever works best for you. In general, refactor PRs should be merged very quickly to minimize merge conflicts.
• I agree with splitting route.shared.ts into smaller files. But there are too many files; many files have only ~10 LOC functions. Instead we can create a src/routing/utils/ that contains self-contained-low-level functions (similarly to src/utils/). The logic of each file in src/routing/utils/ should be self contained. Having many files in src/routing/utils/ is fine but src/routing/ should contain only few files that represent what happens on a high-level.
• Let's keep name conventions: "Route Strings" (instead of "Static Routes"), "Route Functions" (instead of "Functional Routes"), name files should be camelCased and if they contain only one export they should be named after the export.

[brillout]

I suggest the following next steps:

1. Let's move all Vue Router Integration code to a temporary package @vite-plugin-ssr/vue-router (I will add you to the npm org) that lives in this repository in a vue-router/ directory (=> package.json#workspaces.includes('vue-router/')). We can then move it to a separate repository (or not, as we wish).
2. Revert non-essential changes that have been made to src/. E.g. the refactoring of the code.
3. Let's make the test suite go green. If you run npm run test:setup && npm run test you'll see that the test suite fails.

I see the following options for how we expose configuration of the matcher

The Vite plugin is for building which we don't need to change. Instead, AFAICT, the following should work.

// _default.client.js

import { useVueRouter } from '@vite-plugin-ssr/vue-router/client';

// ...

// server/index.js

import { createPageRender } from 'vite-plugin-ssr'
import { useVueRouter } from '@vite-plugin-ssr/vue-router';

const renderPage = createPageRender({ viteDevServer, isProduction, root, customRouting: useVueRrouter });

// ...

src/ would expose functions to override default behavior that @vite-plugin-ssr/vue-router uses. For example:

export { changeRouteStringResolver }

let customResolver
function changeRouteStringResolver(customResolver_) {
  customResolver = customResolver_
}

function resolveRouteString(...args) {
   if( customResolver ) {
      return customResolver(...args)
   }
   // Default behavior
   // ...
}

Looking forward to have Vue Router deeply intergrated!

[gryphonmyers]

Couple of notes:

• I agree with some of your refactoring but let's focus this PR on vue integration. Including refactoring in a PR: makes it harder for me to comprehend what the PR changes, and makes it harder to resolve merge conflicts (which will actually occur after I merge my latest to changes to master).
• We can create a seperate refactor PR before or after this PR, whatever works best for you. In general, refactor PRs should be merged very quickly to minimize merge conflicts.
• I agree with splitting route.shared.ts into smaller files. But there are too many files; many files have only ~10 LOC functions. Instead we can create a src/routing/utils/ that contains self-contained-low-level functions (similarly to src/utils/). The logic of each file in src/routing/utils/ should be self contained. Having many files in src/routing/utils/ is fine but src/routing/ should contain only few files that represent what happens on a high-level.
• Let's keep name conventions: "Route Strings" (instead of "Static Routes"), "Route Functions" (instead of "Functional Routes"), name files should be camelCased and if they contain only one export they should be named after the export.

The thing is, there are parts of this refactor that are required for the integration to work. If you'd like me to recombine some of these files into single-use functions within their respective files, I can do that. I can also help navigate any merge conflicts and fix tests.

The real problem that the refactor seeks to address in order to make the integration possible, is that you have baked the matching logic into the route function. For a Vue Integration to work, we need to rely on their matching logic. This is because I can't parse a Vue route and get back { matchValue: boolean|number, routeParams: Object } in order to do our own matching on the parsed result, nor, I would argue, should I. What we should be doing, and what this refactor accommodates, is allowing us to pass the routes into vue router, then let it use its own matching logic in order to tell us what route we are now on.

For these reasons, your suggestion of exposing changeRouteStringResolver will not suffice. This is why I have been speaking in terms of a matchRoutes function. We need to expose a function that takes all the routes and gives us back a single match.

[gryphonmyers]

I suggest the following next steps:

1. Let's move all Vue Router Integration code to a temporary package @vite-plugin-ssr/vue-router (I will add you to the npm org) that lives in this repository in a vue-router/ directory (=> package.json#workspaces.includes('vue-router/')). We can then move it to a separate repository (or not, as we wish).
2. Revert non-essential changes that have been made to src/. E.g. the refactoring of the code.
3. Let's make the test suite go green. If you run npm run test:setup && npm run test you'll see that the test suite fails.

I see the following options for how we expose configuration of the matcher

The Vite plugin is for building which we don't need to change. Instead, AFAICT, the following should work.

// _default.client.js

import { useVueRouter } from '@vite-plugin-ssr/vue-router/client';

// ...

// server/index.js

import { createPageRender } from 'vite-plugin-ssr'
import { useVueRouter } from '@vite-plugin-ssr/vue-router';

const renderPage = createPageRender({ viteDevServer, isProduction, root, customRouting: useVueRrouter });

// ...

src/ would expose functions to override default behavior that @vite-plugin-ssr/vue-router uses. For example:

export { changeRouteStringResolver }

let customResolver
function changeRouteStringResolver(customResolver_) {
  customResolver = customResolver_
}

function resolveRouteString(...args) {
   if( customResolver ) {
      return customResolver(...args)
   }
   // Default behavior
   // ...
}

Looking forward to have Vue Router deeply intergrated!

1. Sounds good. I'd suggest keeping everything in this repository as a monorepo.
2. I'd like to get more clarity between us on what is considered "non-essential." Please take a look at my previous comment. If you'd like to discuss further perhaps we could take the conversation to Discord?
3. Can do- would like to get point 2 resolved first so we can get consensus on the solution before I fix the tests.

The code structure you're proposing should work, with the caveat that as I said in my previous comment, changeRouteStringResolver will not suffice as the override. I think we're overriding matchRoutes and possibly sortRoutes.

[brillout]

For these reasons, your suggestion of exposing changeRouteStringResolver will not suffice. This is why I have been speaking in terms of a matchRoutes function. We need to expose a function that takes all the routes and gives us back a single match.

changeRouteStringResolver was just an example of how we can make @vite-plugin-ssr/vue-router override default behavior; you're right, we need to override more things.

I'd like to get more clarity between us on what is considered "non-essential."

Mainly the moving of functions into their own file. IIRC reverting this should do it. If there is anything else I can let you know. Just ping me when there are changes on the PRs.

Let me know if you need anything else.

[brillout]

Sounds good. I'd suggest keeping everything in this repository as a monorepo.

👍. I'll add you to the repo.

[gryphonmyers]

Ok! I'll only split out the functions that seem like they're begging for it and let you know when that's complete.

[brillout]

Perfect. I merged my changes to master; as expected there are merge conflicts. See the CHANGELOG.md to see why the changes have been made. As usual, let me know if you have any question.

[gryphonmyers]

@brillout I've recombined some of the routing functions (I looked for anything that was single use and added it back to the file where it was being used). I also merged from master and resolved the merge conflicts, did some various clean up / refactor... and also created a package for vue router as you described. Please have a look.

I suppose the last thing to figure out is this stuff about the route functions, whether we're ending support of some part of that feature to make client routing integration easier.

[gryphonmyers]

@gryphonmyers Let me know if you are busy and I'll finish up the PR myself. Although note that my comments are all about minor things.

Won't be at my computer until Monday. I haven't looked at your previous comments yet, but if you've got ideas about changes you'd like to make, feel free!

[brillout]

👍 I'll wait :-)

[gryphonmyers]

Sorry, pretty slammed with other stuff at the moment. I haven't forgotten about this and will try to get to it ASAP.

[brillout]

No rush, take all the time you need :-)

…

On Tue, May 11, 2021 at 10:48 PM Gryphon Myers ***@***.***> wrote: Sorry, pretty slammed with other stuff at the moment. I haven't forgotten about this and will try to get to it ASAP. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVQRVEG4YXT533WT3VQPLTNGJYRANCNFSM422XVK2A> .

[brillout]

@gryphonmyers Any updates on this? Let me know if anything changed on your side, e.g. if you stopped being interested in using Vue Router with vite-plugin-ssr.

[gryphonmyers]

@brillout nope I just have to deliver a big project with a deadline so have been focusing all my dev time on that. Sorry for the delay, will get back to it when I can.

[brillout]

No worries. Do you have an ETA for that big project? I'm just asking to plan my work on vite-plugin-ssr.

[gryphonmyers]

No worries. Do you have an ETA for that big project? I'm just asking to plan my work on vite-plugin-ssr.

About a month. Since we were so close on this I might try to squeeze in some time before then to wrap it up, maybe on a weekend.

[brillout]

No don't worry, I'll do the wrap up.

Thank you for your wonderful work. I'll let you know if I have questions.

[brillout]

I didn't manage to run the vue-router example, let's wait until you have more time to finish up the PR.

[brillout]

Let me know if you need help merging/rebasing on master. There will be a couple of merge conflicts.

[gryphonmyers]

I didn't manage to run the vue-router example, let's wait until you have more time to finish up the PR.

Ok sorry for the delay - never thought the PR would be open this long

[brillout]

No worries :-)

[gryphonmyers]

OK I just merged from master and I THINK I resolved all the merge conflicts. Still need to run some code and get the tests working.

[gryphonmyers]

@brillout I'm having some issues with the tests - they seem to cause my vue-router package to use an out-of-date (not symlinked) version of vite-plugin-ssr, even though I have it in the package.json as a local path. Any idea what might be causing this? I can confirm an npm install into that directory produces a correct symlink, but running the test command, it somehow gets the bad, non-symlinked copy.

[brillout]

Yes that's because /scripts/link.ts is not aware of your newly created pacakge.

You can add it to getDependers / scripts/helpers/locations.ts.

[brillout]

Btw. have you thought abour our original conversation? I was arguing that using Vue Router with vite-plugin-ssr doesn't make that much sense.

While I agree that vite-plugin-ssr should be flexible, I only had a handful of users asking for a deep Vue Router integration.

Thoughts?

[gryphonmyers]

I mean I've personally seen several people asking how to use it with vue-router and the current answer is you kind of can, but it's complicated to set up and not at all ideal. Nuxt has tight coupling to vue-router (TOO tight if you ask me), but many refugees from that project will expect the integrations it provides. I've already made my points about the advantages of client routing, and your homebrew routing is fine for simple use cases but cannot replace vue-router as it does not integrate with the rendering framework.

[gryphonmyers]

I think the direction we have going here is solid: an opt-in package to integrate a popular rendering framework's companion router painlessly with the plugin's structural conventions. Way more versatile than Nuxt and will be just as powerful if we can ever get this pr merged! I do have a few ideas about further improvements but let's save that for later.

[brillout]

The import RoutingHandler is not used; we can remove it.

[brillout]

The onBeforeRroute() hook should enable us to deeply integrate Vue Router without any change to vite-plugin-ssr's code. Thus closing. Super looking forward to get our Vue Router deep integration work going :-).