Watcher improvements#4509
Conversation
GirlBossRush
added
enhancement
| import { CompilationStats, onLine, OnLineCallback, VSCodeCompileStatus } from "../../src/node/util" | ||
|
|
||
| interface DevelopmentCompilers { | ||
| [key: string]: ChildProcess | undefined |
There was a problem hiding this comment.
This seems to help non-generic functions like Object.entries play nice with the type signature.
| } | ||
|
|
||
| if (!this.hasVerboseLogging) { | ||
| console.log("\n[Watcher]", "Compiler logs will be minimal. Pass --log to show all output.") |
There was a problem hiding this comment.
This helps quite a bit when dealing with the extension compilation which is often quite verbose and inactionable.
There was a problem hiding this comment.
Yes! Basically this hides it unless --log is passed when starting yarn watch? Fantastic DX improvement 🔥
|
✨ Coder.com for PR #4509 deployed! It will be updated on every commit.
|
GirlBossRush
force-pushed
the
watcher-improvements
branch
3 times, most recently
from
58f1c54 to
e969ebb
Compare
Codecov Report
@@ Coverage Diff @@
## main #4509 +/- ##
==========================================
- Coverage 66.03% 65.44% -0.60%
==========================================
Files 30 30
Lines 1640 1658 +18
Branches 330 333 +3
==========================================
+ Hits 1083 1085 +2
- Misses 474 487 +13
- Partials 83 86 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
A couple nits/questions but I feel like this is mostly good to go! 🔥
This is WAY easier to read by the way - bravo 👏
(when I first joined the team, I didn't really understand how this watcher worked. Now I feel like I do!)
Not sure if @code-asher wants to give the final approval but otherwise, I feel like we can get this in today)
| class Watcher { | ||
| private readonly rootPath = path.resolve(__dirname, "../..") | ||
| private readonly vscodeSourcePath = path.join(this.rootPath, "vendor/modules/code-oss-dev") | ||
| private rootPath = path.resolve(process.cwd()) |
There was a problem hiding this comment.
nit: since we moved vscodeSourcePath any reason not to move rootPath as well? That way paths has all paths.
There was a problem hiding this comment.
Unfortunately JS doesn’t let you reference earlier object properties while still defining entries.
| process.stdout.write(message) | ||
| if (!skipNewline) { | ||
| process.stdout.write("\n") | ||
| /** Development web server. */ |
| this.webServer.kill() | ||
| } | ||
|
|
||
| this.webServer = fork(path.join(this.rootPath, "out/node/entry.js"), process.argv.slice(2)) |
| /** Development web server. */ | ||
| private webServer: ChildProcess | undefined | ||
|
|
||
| private reloadWebServer = (): void => { |
There was a problem hiding this comment.
❓ Basically this reloadWebServer kills the webServer if it exists, forks the process (not sure what process.argv.slice(2) is here? (might be nice to name it or add a comment?) and starts up a new child process, logging a message to let us know it started
(and also adding a log for when the process exits)
There was a problem hiding this comment.
process.argv.slice(2) is a bit of a node convention. Since this array contains all the CLI arguments, the first two entries are usually the node runtime and the script name, e.g. node entry.js
There was a problem hiding this comment.
TIL! Thanks for the explainer!
| //#endregion | ||
|
|
||
| const vscode = cp.spawn("yarn", ["watch"], { cwd: this.vscodeSourcePath }) | ||
| //#region Compilers |
There was a problem hiding this comment.
I like this pattern you've started introducing 👏 Does it have a name? Comment regions?
There was a problem hiding this comment.
I wish I could take credit for it but it’s built into VS Code’s region collapsing.
| } | ||
|
|
||
| if (!this.hasVerboseLogging) { | ||
| console.log("\n[Watcher]", "Compiler logs will be minimal. Pass --log to show all output.") |
There was a problem hiding this comment.
Yes! Basically this hides it unless --log is passed when starting yarn watch? Fantastic DX improvement 🔥
| Watcher.log("killing tsc") | ||
| tsc.removeAllListeners() | ||
| tsc.kill() | ||
| this.cleanFiles() |
There was a problem hiding this comment.
this section looks a lot cleaner 🔥
| // Wait for watch-client since "Finished compilation" will appear multiple | ||
| // times before the client starts building. | ||
| if (strippedLine.includes("Starting 'watch-client'")) { | ||
| console.log("[VS Code] 🚧 Compiling 🚧", "(This may take a moment!)") |
There was a problem hiding this comment.
🔥🔥 love the emojis in the logs :D
| "@typescript-eslint/parser": "^5.0.0", | ||
| "audit-ci": "^5.0.0", | ||
| "codecov": "^3.8.3", | ||
| "del": "^6.0.0", |
There was a problem hiding this comment.
nit: should this be a devDependency?
There was a problem hiding this comment.
LOL does this mean my eyesight is going? Apologies!
| ].join("|") | ||
| const re = new RegExp(pattern, "g") | ||
|
|
||
| export type OnLineCallback = (strippedLine: string, originalLine: string) => void |
There was a problem hiding this comment.
👏 appreciate the type extraction and naming to make it easier to read/maintain
- Clean up watcher behaviors.
It's less than a %. I think it's okay for us to ignore this |
GirlBossRush
force-pushed
the
watcher-improvements
branch
from
11fc940 to
f47abdf
Compare
2 participants
This PR addresses feedback listed in #4499 (comment)
Previously, attempting to load VS Code before the compiler all files resulted in some cryptic errors. Our file watcher now detects when VS Code has truly finished compiling and appends a file our server can check for during development.