feat: use LinterHost for linting#1605
Conversation
🦋 Changeset detectedLatest commit: e28e4ae 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.
|
| @@ -3,7 +3,7 @@ | |||
| "tsBuildInfoFile": "node_modules/.cache/tsbuild/info.test.json", | |||
| "rootDir": "src/", | |||
| "outDir": "node_modules/.cache/tsbuild/test", | |||
| "types": [] | |||
| "types": ["node"] | |||
There was a problem hiding this comment.
We need this so that import.meta.dirname is available
| // TODO: remove this; there is a bug in blobReadingMethods - it doesn't respect type from @types/node | ||
| lib: ["DOM"], |
There was a problem hiding this comment.
TODO: file followup issue once this PR merged
| const patchedReaddirSync: typeof fs.readdirSync = (readPath, options) => { | ||
| assert( | ||
| typeof options === "object" && | ||
| options != null && | ||
| Object.keys(options).length === 1 && | ||
| options.withFileTypes === true, | ||
| `ts.sys.readDirectory passed unexpected options to fs.readdirSync: ${JSON.stringify(options)}`, | ||
| ); | ||
| assert( | ||
| typeof readPath === "string", | ||
| "ts.sys.readDirectory passed unexpected path to fs.readdirSync", | ||
| ); | ||
| try { | ||
| fs.readdirSync = originalReadDirSync; | ||
| return host | ||
| .readDirectory(path.resolve(host.getCurrentDirectory(), readPath)) | ||
| .map( | ||
| (dirent) => | ||
| new DirentCtor( | ||
| dirent.name, | ||
| dirent.type === "file" | ||
| ? UV_DIRENT_TYPE.UV_DIRENT_FILE | ||
| : UV_DIRENT_TYPE.UV_DIRENT_DIR, | ||
| readPath, | ||
| ), | ||
| ); | ||
| } finally { | ||
| fs.readdirSync = patchedReaddirSync; | ||
| } |
There was a problem hiding this comment.
Reasoning from #1221:
TypeScript allows users of its Compiler API to provide their own implementation of
ts.System, a collection of host operation abstractions. One of these operations isreadDirectory. At first glance, it may seem roughly equivalent tofs.readdirSync, but in reality it is a full-blown, filterable function with recursion support. Its signature is:readDirectory(directoryPath, extensions, exclude, include, depth).If someone (me) wants to implement their own VFS overlay on top of the disk FS without loosing semantic correctness (the original
readDirectoryhas many quircks), they would have to copy the originalreadDirectoryimplementation from the TypeScript source code. This is because TypeScript doesn't export the internal primitives used byreadDirectory.The total amount of code used involved in the original
readDirectoryimplementation is ~1500 LOC!!! For reference, Volar also ended up copying TypeScript source code.Fortunatelly, I was able to narrow this down to ~50 LOC by patching the
fs.readdirSynccall used inside the originalreadDirectory. This approach is far from perfect, but it's definitely better than copying 1500 LOC of TypeScript source code to Flint.
There was a problem hiding this comment.
TODO: bump typescript-eslint to 8.53.0 so that deps are deduped. I didn't to it because it introduces a new fixer, and the diff would be big.
Line 63 in 79ce5fa
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This looks great to me! I just have a few questions and suggestions about the TS-specific host (putting it in rule-tester vs. ts; having it at all).
|
|
||
| const host = createEphemeralLinterHost( | ||
| createDiskBackedLinterHost(process.cwd()), | ||
| ); |
There was a problem hiding this comment.
[Non-Actionable] Hmm, runCliOnce is called by runCliWatch. runCliWatch might want to set up a longer-lived linter host... but I don't think we should block this PR on that. That's a nice-to-have optimization for now IMO.
There was a problem hiding this comment.
Yep, we could also reuse disk-backed LinterHost's watchDirectory there. I'll file a followup.
packages/rule-tester/src/host.ts
Outdated
| defaultCompilerOptions?: Record<string, unknown>; | ||
| } | ||
|
|
||
| export function createRuleTesterTSHost( |
There was a problem hiding this comment.
[Refactor] 🤔 I'm not sold on making a language-specific host that's so hardcoded. I'd think many languages will eventually want to have their own customizations. My intuition is it'd be cleanest to have the RuleTester constructor take in whatever settings are needed.
If I'm understanding it right, there are roughly four customizations done here:
- Setting the dirname to a directory that doesn't exist
- Creating an ephemeral + disk-backed host for that directory, then a VFS host on top of that
- Upserting some TypeScript-specific files
- Stopping fs stat calls from reaching any
tsconfig.json
Is that about right?
If so: how about the RuleTester...
- Always creates a host set to completely virtual directory - so no "polluting" it with files on disk not explicitly declared
- Has a "default files" constructor option to pre-populate that virtual directory with files such as tsconfigs
- Allows tests to override those files as needed
(where 3 is the #621 followup)
?
There was a problem hiding this comment.
If I'm understanding it right, there are roughly four customizations done here:
- Setting the dirname to a directory that doesn't exist
- Creating an ephemeral + disk-backed host for that directory, then a VFS host on top of that
- Upserting some TypeScript-specific files
- Stopping fs stat calls from reaching any
tsconfig.jsonIs that about right?
Yes! Although, on second thought, preventing stat calls is not really useful here since the overlay VFS always has a tsconfig.json.
If so: how about the
RuleTester...
- Always creates a host set to completely virtual directory - so no "polluting" it with files on disk not explicitly declared
Do you mean createVFSLinterHost({ baseHost: createDiskBackedLinterHost(), cwd: 'some-virtual-directory' })?
If so, only TS-based languages need to read disk (for node_modules only). All other languages should be fine with createVFSLinterHost() alone. They don't need to be backed by disk. So if we move this to the RuleTester constructor, we probably should introduce a new option - allow/disallow disk access.
- Has a "default files" constructor option to pre-populate that virtual directory with files such as tsconfigs
So every plugin that uses TS language would have to provide tsconfig.json + tsconfig.base.json individually? Or maybe we can export these two from TS plugin to avoid duplication...
I was hoping to keep things as composabile as possible, so RuleTester users can do whatever they want with LinterHost. For example, someone might want to test a situation where a peer dependency is missing on disk. The only way to do that is to provide an overlay that blocks access to individual files. If we construct LinterHost entirely inside RuleTester, it won't be as composable.
That said, it may turn out that this use case never comes up. And we can always move to this composable format later if needed!
There was a problem hiding this comment.
Do you mean
Yes, that 👍
So every plugin that uses TS language would have to provide
tsconfig.json+tsconfig.base.jsonindividually?
That's what I was thinking, yes. I don't think they'll always be the same - e.g. the browser and JSX plugins will want DOM types, but I don't think the Node.js and Performance ones will.
That said, it may turn out that this use case never comes up. And we can always move to this composable format later if needed!
Same thought, yeah. It'd be nice in theory to be composable but for now I think it'll be easier for us to start with a smaller, more "there's one main way to do it" API.
There was a problem hiding this comment.
Same thought, yeah. It'd be nice in theory to be composable but for now I think it'll be easier for us to start with a smaller, more "there's one main way to do it" API.
👍
Then this PR is ready for the review, I think
UPD: oops, let me resolve merge conflicts first
There was a problem hiding this comment.
Awesome!
If so, only TS-based languages need to read disk (for
node_modulesonly).
Yeah, this "one particular language has one particular need" has been bugging me. I see why you set this up to allow them to do so (and why it was required) but I'd love to have a followup to smooth that little wrinkle out. Maybe node_modules/ should just always be available ask a disk-backed part of the tester hosts? 🤷 This is definitely fine -better, really- as a followup discussion/issue IMO.
There was a problem hiding this comment.
fixes #73
I think this gets most of the way there, but there are still both prepareFromDisk and prepareFromVirtual. I was thinking resolving the issue would mean actually merging those two into one API. Like a unified prepare() or prepareFile() kind of name. And then later on, once those are simplified, it'd be easier to take a next step towards #1291.
export const textLanguage = createLanguage<TextNodes, TextFileServices>({
about: {
name: "Text",
},
createFileFactory: (host) => {
return {
prepareFile: (data) => {
const sourceText =
data.sourceText ?? host.readFile(data.filePathAbsolute);
// (or maybe return undefined, or return the Error, ...)
if (!sourceText) {
throw new Error(`Could not find file '${data.filePathAbsolute}'.`);
}
return {
file: createTextFile({
...data,
sourceText,
}),
};
},
};
},
});Does that differ from where you see this all evolving?
Not a blocker IMO, just a note that if this isn't done in this PR I think we'll have more work to do to streamline the file prep APIs.
There was a problem hiding this comment.
Yeah, we have an issue for this #1500. I decided to split it out because it would require touching all language file factories.
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
🔥🔥🔥 Fantastic! I'm pumped we're moving so solidly into a host + virtual files setup like this. Not just because of the great perf speedup mentioned in the OP, but because this is really important work for filling out Flint's core fs architecture. Really solid work @auvred, nicely done!
If anybody from @flint-fyi/committer or anybody else from @flint-fyi/maintainer has time to look this over, that'd be great. But I recognize this has been open for a few days and is pretty deep, entrenched work that also is upstream of some more stuff. I think it's fine to merge whenever you want to @auvred.
packages/rule-tester/src/host.ts
Outdated
| defaultCompilerOptions?: Record<string, unknown>; | ||
| } | ||
|
|
||
| export function createRuleTesterTSHost( |
There was a problem hiding this comment.
Awesome!
If so, only TS-based languages need to read disk (for
node_modulesonly).
Yeah, this "one particular language has one particular need" has been bugging me. I see why you set this up to allow them to do so (and why it was required) but I'd love to have a followup to smooth that little wrinkle out. Maybe node_modules/ should just always be available ask a disk-backed part of the tester hosts? 🤷 This is definitely fine -better, really- as a followup discussion/issue IMO.
| for (const oldFile of linterHost.vfsListFiles().keys()) { | ||
| if (oldFile !== filePathAbsolute) { | ||
| linterHost.vfsDeleteFile(oldFile); | ||
| } | ||
| } | ||
| linterHost.vfsUpsertFile(filePathAbsolute, code); |
There was a problem hiding this comment.
Do we need the !== check if we upsert immediately after? Is upserting just cheaper than inserting?
There was a problem hiding this comment.
Deleting and then inserting triggers the watcher twice. If we upsert instead, the watcher will be called only once.
| host: createTypeScriptServerHost(host), | ||
| }); | ||
|
|
||
| function prepareFile(data: FileAboutData) { |
There was a problem hiding this comment.
Is there a particular reason to put this as a closure rather than keeping this a separate file? IMO it makes it harder to see what's going on in the function. Then again this is going away soon, so probably not worth refactoring.
There was a problem hiding this comment.
Yeah, there will be a lot of stuff happening for Volar integration, so it will probably get refactored anyway.
lishaduck
left a comment
There was a problem hiding this comment.
LGTM
I left a few nits but I don't think they're necessary to address, especially given how much more churn this'll be going through in the near future.
|
I don't think there's anything left to merge, so... 🟩 |
|
🥳 |

PR Checklist
ScriptKind.TSfor non-TS languages #1142status: accepting prsOverview
I could probably split this up into two PRs: one to pass
LinterHosteverywhere around and another to remove@typescript/vfs. But the diff is a moderate size, so I think this is fine.Nice bonus: on my machine, running
pnpm vitest --run packages/tstakes:main~40s