feat(core): introduce LinterHost#1246
Conversation
🦋 Changeset detectedLatest commit: 0e6d382 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 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.
|
auvred
added
the
package: core
| using _ = host.watchFile(filePath, onEvent, 10); | ||
|
|
||
| await sleep(50); | ||
|
|
||
| fs.writeFileSync(filePath, "first"); |
There was a problem hiding this comment.
sleep(50) is used to account for Windows slowness. Sometimes it can't set up the watchers in time before fs.writeFileSync is called.
There was a problem hiding this comment.
You've got to love having to develop on Windows... 🙃
Suggestion: how about moving the await sleep(50);s to a helper function like sleepIfNeeded() that only waits if the OS is Windows? That way we don't get 50ms delays on Linux and Mac.
There was a problem hiding this comment.
It appears that when tests are run for the entire workspace, they're executed in parallel, and CI needs these timeouts too: https://github.com/flint-fyi/flint/actions/runs/20680548741/job/59374155423#step:5:770 🤷
With these timeouts, it runs locally in ~3.5s, so it's not that slow after all.
| let unwatched = false; | ||
| const unwatchSelf = () => { | ||
| unwatched = true; | ||
| watcher.close(); | ||
| }; |
There was a problem hiding this comment.
This is done for potential race conditions that cannot be reliably tested, where the watcher fires twice for the same file.
|
I'm on MacOS, I pulled your branch, everything builds fine but yeah unfortunately the tests do fail. Dumping some screenshots, let me know if you need me to look into anything else further or try some things out for you 🤔 Edit: removed my sad unhelpful screenshots 😂 |
Did you build it first? Those are all module-not-found errors which shouldn't be popping up from these changes at least 🤔 |
|
Thanks a lot! But these failures are unrelated to the linter host tests. Did you run |
|
Whoops, that's embarrassing. I may have run the tests before the node_module installs finished 😅 Reran them now and I see there's just 1 error. 
|
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
No blockers from me, this is a great clean start. Nice! 🥇
TIL about the case swapping trick, very nifty.
JoshuaKGoldberg
added
the
1 approval
lishaduck
left a comment
lishaduck
left a comment
There was a problem hiding this comment.
There's certainly followups needed, but I think we can save them for a followup to unblock #1179?
Ideally, we should setup a test matrix accross these major platforms. Also, createDiskBackedLinterHost.test.ts probably should be converted to an integration test, since it's impure (it creates ephemeral directory in the nearest node_modules).
4 participants
PR Checklist
status: accepting prsOverview
fs.watchhas a LOT of quirks and inconsistencies across different platforms. I tried to cover them all, although I definitely missed some edge cases. Also since watching is asynchronuous and callback-based, there may be race conditions like this one:fs.watchis removed in favor offs.watchFilefs.watchFileis establishedHowever, this is super rare case, I guess. As far as I understand,
ts.sysdoesn't handle this case either.I tested this on Linux and in a Windows VM. It would be nice if someone with macOS could run the tests on their machine as well. Ideally, we should setup a test matrix accross these major platforms. Also,
createDiskBackedLinterHost.test.tsprobably should be converted to an integration test, since it's impure (it creates ephemeral directory in the nearestnode_modules).