-
-
Notifications
You must be signed in to change notification settings - Fork 102
feat: volar plugins #609
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
base: main
Are you sure you want to change the base?
feat: volar plugins #609
Conversation
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
- Coverage 61.72% 59.85% -1.87%
==========================================
Files 32 36 +4
Lines 3135 3333 +198
Branches 580 601 +21
==========================================
+ Hits 1935 1995 +60
- Misses 1194 1330 +136
- Partials 6 8 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
// TODO: Do we want to apply this to EVERY .vue file or only to components that the user wrote themselves? | ||
|
||
const relativeFilePath = ctx.compilerOptions.baseUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will ctx.compilerOptions.baseUrl
always equal the VueRouter
plugin's root
? If not, this might not always work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question
…olar plugin sfc-route-blocks
@@ -0,0 +1,7 @@ | |||
<template> | |||
<RouterView name="default" /> | |||
<RouterView name="a" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are typing these too? Pretty neat! It might be worth to adapt types in vue router too first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was just an experiment, but not sure if I actually finished this. I'll check in a few days!
|
||
const plugin: VueLanguagePlugin = (ctx) => { | ||
const RE = { | ||
USE_ROUTE: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are regexes the recommended way? Isn't there an ast to traverse instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If AST is truly possible, that would be better, but I noticed other Volar plugins usually use regexes too. Maybe @KazariEX could give us his two cents here :)
|
||
/* | ||
Future ideas: | ||
- Enhance typing of `useRouter().currentRoute` and `$router.currentRoute` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better not to because the router instance can be passed to functions and it's safer to keep that route as any route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on how this is different from passing a typed useRoute()
or $route
to functions like composables? The function you pass it to can choose to either accept a generalize route/router, like ReturnType<typeof useRouter>
/ReturnType<typeof useRoute>
or make it more specific by requiring the passed router/route with a generic, like ReturnType<typeof useRoute<'blogs'>>
. As far as I can remember, useRouter
does not accept a generic (yet), but it could and then it should be fine, I think?
return | ||
} | ||
|
||
// TODO: Do we want to apply this to EVERY .vue file or only to components that the user wrote themselves? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want to apply this to page components. The question might be how do we pass those paths from the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a file isn't matched, because it's not in the generated RouteFileInfoMap
, the route is generalized, I believe. But it needs extensive testing.
*/ | ||
export type GetPossibleRouteNamesByFilePath<T extends string> = T extends keyof RouteFileInfoMap | ||
? RouteFileInfoMap[T]['routes'] | ||
: keyof import('vue-router/auto-routes').RouteNamedMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the dynamic import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it had something to do with interface declaration merging. Maybe test if it works without the dynamic import when you extend the RouteNamedMap
elsewhere in a project.
"export interface RouteFileInfoMap { | ||
'src/pages/index.vue': { | ||
routes: '/', | ||
views: never, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't they be default
in most cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a route file does not have any child routes, you shouldn't be using <RouterView>
, right? Or are there edge cases I'm missing here?
node: TreeNode, | ||
options: GenerateRouteFileInfoMapOptions | ||
): string { | ||
const children = node.children.size > 0 ? node.getSortedChildren() : null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to update the branch and use the new deep version of this
replace( | ||
embeddedCode.content, | ||
'{', | ||
'{\n "$schema": "https://raw.githubusercontent.com/posva/unplugin-vue-router/main/route.schema.json",' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
|
||
// TODO: Do we want to apply this to EVERY .vue file or only to components that the user wrote themselves? | ||
|
||
const relativeFilePath = ctx.compilerOptions.baseUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question
? relative(ctx.compilerOptions.baseUrl, fileName).replaceAll('\\', '/') | ||
: fileName | ||
|
||
const routeNameGetter = `import('vue-router/auto-routes').GetPossibleRouteNamesByFilePath<'${relativeFilePath}'>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we should be able to just pass the name of the route associated with the file
<route>
block IntelliSense intosrc
, added it to the building process and added it to exportsvueCompilerOptions.plugins
)useRoute
and$route
typingsvueCompilerOptions.plugins
)Future
<RouterView>
slots regarding named views