Conversation
🦋 Changeset detectedLatest commit: 09abd33 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 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.
|
|
I'll review the code in a moment (want to rebase the host work first), but initial thoughts:
Obviously I haven't read the code yet to know if you lied in there 😛 but you did a very good job explaining. It made sense as I read and and was a nice and linear. Don't quiz me though (yet).
Agreed. Haven't looked at it yet but I'm 100% in favor of getting the hacky version in now and rewriting the rest of the codebase to work with #1253 (comment) later.
I just want to say that's really impressive :) |
lishaduck
left a comment
There was a problem hiding this comment.
Only took me 3 weeks to get around to "review[ing] the code in a moment" :)
Looks good! Some of it goes over my head but I tried to leave comments where it seemed like it could be clearer.
I'll just leave these as comments, I don't think they're blocking but I'm not confident enough to go full blown approval yet.
| SourceScript as VolarSourceScript, | ||
| } from "@volar/language-core"; | ||
| import type { TypeScriptServiceScript as VolarTypeScriptServiceScript } from "@volar/typescript"; | ||
| // eslint-disable-next-line no-restricted-syntax |
There was a problem hiding this comment.
lol that's funny. Should probably fix that at some point 😅
| assert( | ||
| globalTyped._flintVolarLanguageState == null, | ||
| `Two different versions of ${packageJson.name} are imported: ${packageJson.version} and ${globalTyped._flintVolarLanguageState?.packageVersion}`, | ||
| ); |
There was a problem hiding this comment.
This is the same as in the typescript plugin, b/c they both need to be singletons, correct?
I am mildly worried about 1) peer dependencies conflicts, we could potentially get two different versions with differing peers and 2) actually accessing peers in Yarn/hermetic pnpm, but I think we can see if it's an issue in practice before scrambling. Packaging is complicated 😅
There was a problem hiding this comment.
This is the same as in the typescript plugin, b/c they both need to be singletons, correct?
Yeah, these are consequences of relying on singletone patched typescript package and global import side effects.
Maybe we can refactor all of this to use AsyncLocalStorage some day...
And also using different versions of languages because of peer deps mess goes against our TS project service approach, so this is another reason to ban using two versions of the same language.
| visitors.SourceFile?.(sourceFile, visitorServices); | ||
| // Visit only statements that have a mapping to the source code | ||
| // to avoid doing extra work | ||
| Statements: for (const statement of sourceFile.statements) { |
There was a problem hiding this comment.
Don't we have a lint against labels? (this usage doesn't actually seem that problematic to me, just concerned lints might be broken again.
There was a problem hiding this comment.
🤔 Both ESLint and Flint are happy
There was a problem hiding this comment.
Lol. AI notwithstanding, I think this language feature has become so esoteric that only people who really want it use it...
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Glorious. I agree, let's ship. 🚀
|
Btw, @JoshuaKGoldberg, since |
|
|
||
| const fileExtToFlintPlugin: Record<string, string> = { | ||
| ".astro": "@flint.fyi/astro", | ||
| ".ember": "@flint.fyi/ember", |
There was a problem hiding this comment.
Do we intend on having an ember plugin? I was using Ember as an example of a language we probably wouldn't support in-house.
There was a problem hiding this comment.
Glint already has Volar plugin, so we can add support for Ember in under 50 LOC https://github.com/auvred/golar/blob/ad32aa1151572251b0b68a2d823d3c0ebab2418a/packages/ember/src/bin.ts
I think it won't hurt?
There was a problem hiding this comment.
OOPS, just noticed this: there is no .ember file extension xD. It's .{gts,gjs}
| ) { | ||
| // TODO: suggestions, fixes | ||
| context.report({ | ||
| ...report, |
There was a problem hiding this comment.
Is there anything left to spread then?
There was a problem hiding this comment.
data, message, dependencies and so on :)
| } | ||
| function sourceCodeRange(range: CharacterReportRange): CharacterReportRange { | ||
| return { | ||
| begin: -range.begin, |
There was a problem hiding this comment.
Wait why is it negative if it's an actual range?
There was a problem hiding this comment.
Rule runtime tries to adjust all reported ranges with
file.adjustReportRange(if present)Now a little hack: we want to have an ability to do both:
- Allow
@flint.fyi/tsrules seamlessly report errors on generated virtual text- Allow
@flint.fyi/vuerules to report errors on source text
Right now we have only one reporting channel:ruleRuntime.report({ message, range }).
So, for case 1., rules just callcontext.report. However, for case 2., rules callreportSourceCode(context, { message, range }).reportSourceCodeis exported from@flint.fyi/volar-language.
reportSourceCodenegates the beginning of the range, so that lateradjustReportRangecan distinguish between case 1. and 2.
This is a first step, it's later negated back to positive by adjustReportRange
| { | ||
| "name": "@flint.fyi/volar-language", | ||
| "version": "0.16.0", | ||
| "version": "0.0.1", |
Mannnn I really don't know what the right strategy is. It's obnoxious. Ludicrous. JoshuaKGoldberg/create-typescript-app#2370 if anybody wants to help me learn these things. |
|
Okay, I looked through the changes one more time, and I think it's time to |
PR Checklist
status: accepting prsOverview
This is a very complex PR and it's even hard to properly explain it in a linear order because things interact in weird ways here, so I'll try to explain it in several steps.
Brief overview:
We have two sets of global variables: in
@flint.fyi/typescript-language(used by@flint.fyi/volar-language) and in@flint.fyi/volar-language(used by@flint.fyi/{astro,svelte,vue}-language). These variables are filled in reverse order by downstream packages when imported (@flint.fyi/vue-language->@flint.fyi/volar-language->@flint.fyi/typescript-language). Later they're used in normal order (@flint.fyi/typescript-language->@flint.fyi/volar-language->@flint.fyi/vue-language).We have several concepts:
VolarCreateFile- callback which is registered in@flint.fyi/typescript-languagefrom@flint.fyi/volar-languageby callingsetVolarCreateFile.Its purpose is to create a Flint's file for an extension language.
VolarLanguagePluginInitializer- contains Volar language plugins andVolarBasedLanguageCreateFile.Plugin initializers are registered globally when Volar-based Flint language is created.
Every extension language (
@flint.fyi/{astro,svelte,vue}-language) provides its own Volar language plugins andVolarBasedLanguageCreateFile.VolarBasedLanguageCreateFile- function that returns extension language-specific info so that@flint.fyi/volar-languageis able to create a Flint file.Volar-based Flint language - meta language that is used to create actual extension language (e.g.,
@flint.fyi/vue-languagecallscreateVolarBasedLanguage).This language is not used anywhere in Flint runtime.
It has two purposes:
VolarLanguagePluginInitializergloballyThese rules use
typescriptLanguageas their language (more on this later).Volar language - orchestrator of several Volar language plugins and Volar scripts. Both Svelte and Vue plugins can co-exist. Volar language decides which one should handle
.svelteand.vue.Volar language plugin - its responsibility is to generate "service script"
Volar's "source script" - wrapper around file's original source text
Volar's "service script" - wrapper around file's generated virtual text
Volar's "mapping" - maps regions of source text to the corresponding regions of generated virtual text
Now, the initialization order (I'll use Vue language for illustration, but this applies to Astro and Svelte as well):
@flint.fyi/vueinflint.config.ts@flint.fyi/vuecontains rules that use@flint.fyi/vue-languagelanguage@flint.fyi/vue-languageregisters.vueextension through@flint.fyi/ts-patchand creates avueLanguagewithcreateVolarBasedLanguage(it passesVolarLanguagePluginInitializerthere)createVolarBasedLanguageregistersVolarLanguagePluginInitializerglobally@flint.fyi/volar-languagesets global TS program creation proxy via@flint.fyi/ts-patch@flint.fyi/volar-languagecallssetVolarCreateFileexported from@flint.fyi/typescript-languagewithVolarCreateFilecallbacksetVolarCreateFilesaves this callback globallyAt this point initialization is done (linting hasn't started yet!)
Now,
@flint.fyi/corestarts collecting files for linting:Since Volar-based Flint language rules specify
typescriptLanguageas their language,@flint.fyi/corecallstypescriptLanguage.createFile({ filePath: 'myfile.vue' })typescriptLanguagecallsservice.openClientFile('myfile.vue')This creates TS program internally and
@flint.fyi/volar-languageintercepts program creation via@flint.fyi/ts-patchIt takes
VolarLanguagePluginInitializers (including a@flint.fyi/vue-language's one) and evaluates them in order to get two things:VolarBasedLanguageCreateFile- customizations provided by@flint.fyi/vue-languageAlso, it attaches
VolarBasedLanguageCreateFileto Volar language plugin (__flintCreateFile), so that later we can access it.Then, it attaches Volar language to the proxied TS program (
__flintVolarLanguage)At this moment proxied TS program is a fully functioning program that contains
.vuefiles with generated virtual codetypescriptLanguagedetects that.vueextension is not natively supported by TypeScript and hands off file creation to theVolarCreateFilecallback. SincetypescriptLanguagedoesn't know anything about attached__flintVolarLanguageand__flintCreateFileproperties, it just passests.Programandts.SourceFileto theVolarCreateFile.VolarCreateFile:__flintVolarLanguagefrom thets.Programts.SourceFile's file name__flintCreateFilefrom the Volar language plugin attached to the "source script"__flintCreateFilewithts.Program,ts.SourceFile, Volar language, "source script" and "service script"__flintCreateFile(provided by@flint.fyi/vue-language) (PR - feat: introduce Vue language #2316):@vue/compiler-domin order to get Vue AST<template>extraServiceswith Vue-specific extras, such as Vue AST (they're passed to rules later)Now, back to the
VolarCreateFile. It returns Flint file with:adjustReportRange,__volarServices.runVisitors, and__volarServices.getDiagnostics(more on them later)Now, back to the
typescriptLanguage.createFile. It returns file created byVolarCreateFile(withSymbol.disposeto cleanup opened TS file)At this point we have initialized Flint file.
Now, Flint starts actual linting:
It calls
typescriptLanguage.runFileVisitorstypescriptLanguage.runFileVisitorstries to accessfile.__volarServices. If they're present, it hands off visiting to the__volarServices.runVisitors, otherwise it defaults to commonts.SourceFilevisiting.__volarServices.runVisitorsstarts visitingts.SourceFile. Remember that this is generated virtual text ("service script"). In order to avoid doing extra work it visits only statements that contain at least one "mapping" inside them.When some visitor decides to report an error, it calls
ruleRuntime.reportRule runtime tries to adjust all reported ranges with
file.adjustReportRange(if present)Now a little hack: we want to have an ability to do both:
1. Allow
@flint.fyi/tsrules seamlessly report errors on generated virtual text2. Allow
@flint.fyi/vuerules to report errors on source textRight now we have only one reporting channel:
ruleRuntime.report({ message, range }).So, for case 1., rules just call
context.report. However, for case 2., rules callreportSourceCode(context, { message, range }).reportSourceCodeis exported from@flint.fyi/volar-language.reportSourceCodenegates the beginning of the range, so that lateradjustReportRangecan distinguish between case 1. and 2.If
file.adjustReportRangedetects that the range is reported on source text, it returns it as-is. Otherwise it uses Volar "mappings" to translate range from "service script" positions to "source script".That's it!!!
I know that there are a lot of questionable hacks, however given #1253 (comment), we would have to refactor a large part of
@flint.fyi/core. We can design this refactor these hacks in mind.I think it's better to merge these hacks, because I'm tired of re-syncing Vue support with
@flint.fyi/corechanges every few weeks (a decent portion of this PR has been living locally on my machine since October xD)Now this should automagically work!!!