Conversation
🦋 Changeset detectedLatest commit: 3be92f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| ] | ||
| }, | ||
| "packages/vue": { | ||
| "ignoreDependencies": ["vue"] |
There was a problem hiding this comment.
It's required for tests (virtual code for .vue files contains vue package imports)
| export interface VueServices { | ||
| vueServices: { |
There was a problem hiding this comment.
[Naming] 🤔 this nesting of two things both called "Vue services" is odd. I think it'll be confusing to people trying to read & understand them. Especially folks coming into linting/Flint anew from the Vue side.
I also can't quickly think of a better property name than vueServices for the nested object containing Vue services. Maybe that's a sign that these all make sense as direct properties of VueServices, not nested underneath an object? That would simplify the overall structure of things.
Though, given the comment about Partial<FileServices>, maybe that's not doable? If so - how about calling this vue instead of vueServices? That would signal that it's the "Vue" area of "services".
WDYT?
There was a problem hiding this comment.
[Naming] 🤔 this nesting of two things both called "Vue services" is odd. I think it'll be confusing to people trying to read & understand them. Especially folks coming into linting/Flint anew from the Vue side.
+1 on renaming. Just plain vue property would make more sense
Maybe that's a sign that these all make sense as direct properties of
VueServices, not nested underneath an object? That would simplify the overall structure of things.Though, given the comment about
Partial<FileServices>, maybe that's not doable?
Yeah, answered in #2316 (comment)
If we make these properties top-level, then rule authors will have to do null checks for every individual property rather than just context.vue != null
| } | ||
|
|
||
| type VueCodegen = | ||
| typeof tsCodegen extends WeakMap<WeakKey, infer V> ? V : never; |
There was a problem hiding this comment.
[Aside] I cannot wait for the blog post explaining this architecture 😂 it's going to cause a lot of conversations around tooling folks.
| typeof configFilePath === "string" | ||
| ? createVueParsedCommandLine( | ||
| ts, | ||
| ts.sys, |
There was a problem hiding this comment.
[Question] Shouldn't this also be using options.host in some way? (and the one too)
There was a problem hiding this comment.
There was a problem hiding this comment.
Ohh, nice catch!!
Even though options.host is wrapped like 5 times in TS internals, its FS-related methods still end up in our LinterHost, I updated this place to prefer options.host when it's non-null (always)
packages/vue/src/rules/vForKeys.ts
Outdated
| if (vueServices == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Refactor] I now see the disadvantage of the Partial<FileServices> in createVolarBasedLanguage... is this really necessary? Can we just say:
export function createVolarBasedLanguage<FileServices extends object>(
initializer: VolarLanguagePluginInitializer<FileServices>,
): Language<
TypeScriptNodesByName,
- Partial<FileServices> & TypeScriptFileServices
+ FileServices & TypeScriptFileServices
> {?
There was a problem hiding this comment.
Ah, the problem is that we want Vue rules to lint both .vue and .ts files!
If we remove Partial from there, we will basically limit rules created from vueLanguage to lint only .vue files since Vue services are available only there.
There was a problem hiding this comment.
Almost yes, but no. Currently, Vue is like a superset of TS. Both TS and Vue rules can lint the generated virtual TypeScript code, but only Vue rules can access Vue services (Vue AST) when they're linting .vue files (they can't access them when they're linting .ts files). 1253 is more about cross-language/cross-file linting
There was a problem hiding this comment.
Ok, after a few read-thrus of your comment, do Vue rules also run on .ts files? If not I don't know how to understand that comment. (so #1253 (comment) but the future is sorta now)
There was a problem hiding this comment.
Ok, after a few read-thrus of your comment, do Vue rules also run on .ts files?
Yes, exactly! .ts files can also contain Vue-specific code, for example asyncComputed rule - https://github.com/auvred/flint/blob/5fee75d714f36bce5363a46c9c9d53cf8251da17/packages/vue/src/rules/asyncComputed.test.ts
There was a problem hiding this comment.
Ooooh, cool! Yeah I didn't get that. I'll go re-read and see where I missed that.
There was a problem hiding this comment.
Ok, it's still a little fuzzy around the edges but I think I get the general idea, maybe?
There was a problem hiding this comment.
I'll write a bit more detailed explanation tomorrow 🤝
There was a problem hiding this comment.
So, how this works:
First, we have TS rules (from @flint.fyi/ts). These rules can lint pure TypeScript code only. Thanks to Volar, .vue files, when imported into TS program, are converted to virtual generated TypeScript code.
So:
- when TS rule lints
.tseverything works as before - when TS rule lints
.vue, it lints the virtual generated code, reports errors on it, and our linter engine maps these errors back to their original locations in Vue SFC
What happens when we create a new rule using language from createVolarBasedLanguage:
- when Vue rule lints
.ts, it lints it with the same context/services as TS rule, everything works exactly the same here - when Vue rule lints
.vuefile, similar to the TS rule, it can report errors on the virtual generated code (reports then mapped to the original locations), or it can accessvueservices from the context. In this case, Vue rule can visit the original AST of the Vue SFC, optionally map its location to the generated code (to get some type information), and report the errors with the original Vue SFC locations, in this case these locations are not mapped back since they're already valid.
Hope this helps!
| const toGeneratedLocation = (sourceLocation: number) => { | ||
| for (const [loc] of vueServices.map.toGeneratedLocation( | ||
| sourceLocation, | ||
| )) { | ||
| return loc; | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| const toGeneratedLocationOrThrow = (sourceLocation: number) => { | ||
| return nullThrows( | ||
| toGeneratedLocation(sourceLocation), | ||
| "Unable to map source location to generated location", | ||
| ); | ||
| }; |
There was a problem hiding this comment.
[Refactor] This strikes me as utilities that most-to-all Vue rules will need. I'm guessing they'll end up in a utils directory sooner or later.
There was a problem hiding this comment.
For sure! But I think we need to write several more rules, so that we can better understand the appropriate shape for these utilities
| @@ -0,0 +1,52 @@ | |||
| { | |||
| "name": "@flint.fyi/vue", | |||
There was a problem hiding this comment.
Oh, and: should this be split into the language & rules plugin the way others are too?
| "name": "@flint.fyi/vue", | |
| "name": "@flint.fyi/vue-language", |
There was a problem hiding this comment.
It is! I missed that as well my first readthru 😂
There was a problem hiding this comment.
🤔 But this PR already has both @flint.fyi/vue-language and @flint.fyi/vue? Or am I missing something?
| typeof configFilePath === "string" | ||
| ? createVueParsedCommandLine( | ||
| ts, | ||
| ts.sys, |
There was a problem hiding this comment.
| } | ||
| }; | ||
|
|
||
| // TODO: add vue: listeners to the language |
There was a problem hiding this comment.
I don't know what this means but cool. (aka what's vue:. Can I get like a 2-second vue crash-course? I think that might help)
There was a problem hiding this comment.
Oops, sorry bad phrasing... I meant this - #1163 (comment)
The unclear part is: is it OK to call all TS listeners before and then all vue: listeners after? Probably yes, because we don't have any other options. I'll file the followup issue
|
Okay I think I addressed almost everything! I also want to say thank you both for diving into this and catching so many things here! I know how much brain hurts after trying to understand how all this Volar/Vue stuff works (I spent like several months digging in this, so that's no longer the case for me, but I remember that a few years ago I tried to implement all this as an ESLint plugin and I gave up after two days in a complete desperation because my brain melted 🫠) |
|
|
Even though I'm bad at writing, I'll try to explain everything in details in #2297 |
packages/vue-language/package.json
Outdated
| "test": "vitest --project vue-language" | ||
| }, | ||
| "dependencies": { | ||
| "@flint.fyi/core": "workspace:^", |
There was a problem hiding this comment.
This can just be a devDependency, it's type-only and not exposed (oh how I want a good public/private types detector...).
There was a problem hiding this comment.
Oh wow! What a catch... I can't even imagine how you noticed this
|
Okay, the time has come... |
PR Checklist
status: accepting prsOverview