Watcher improvements #4509

GirlBossRush · 2021-11-15T00:31:34Z

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.

GirlBossRush · 2021-11-15T00:32:35Z

ci/dev/watch.ts

+import { CompilationStats, onLine, OnLineCallback, VSCodeCompileStatus } from "../../src/node/util"
+
+interface DevelopmentCompilers {
+  [key: string]: ChildProcess | undefined


This seems to help non-generic functions like Object.entries play nice with the type signature.

GirlBossRush · 2021-11-15T00:34:26Z

ci/dev/watch.ts

+    }
+
+    if (!this.hasVerboseLogging) {
+      console.log("\n[Watcher]", "Compiler logs will be minimal. Pass --log to show all output.")


This helps quite a bit when dealing with the extension compilation which is often quite verbose and inactionable.

Yes! Basically this hides it unless --log is passed when starting yarn watch? Fantastic DX improvement 🔥

github-actions · 2021-11-15T00:39:29Z

✨ Coder.com for PR #4509 deployed! It will be updated on every commit.

Host: https://codercom-mfswq7vok-codercom.vercel.app/docs/code-server
Last deploy status: success
Commit: 4b38316
Workflow status: https://github.com/cdr/code-server/actions/runs/1482624851

codecov · 2021-11-15T01:23:49Z

Codecov Report

Merging #4509 (4b38316) into main (5fe16be) will decrease coverage by 0.59%.
The diff coverage is 25.00%.

@@            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

Impacted Files	Coverage Δ
src/node/routes/vscode.ts	`46.93% <16.66%> (-3.07%)`	⬇️
src/node/util.ts	`72.63% <27.77%> (-4.79%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fe16be...4b38316. Read the comment docs.

jsjoeio

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)

jsjoeio · 2021-11-18T18:42:49Z

ci/dev/watch.ts

 }

 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())


nit: since we moved vscodeSourcePath any reason not to move rootPath as well? That way paths has all paths.

Unfortunately JS doesn’t let you reference earlier object properties while still defining entries.

Ahhhh got it! Disregard 👍

jsjoeio · 2021-11-18T18:43:09Z

ci/dev/watch.ts

-    process.stdout.write(message)
-    if (!skipNewline) {
-      process.stdout.write("\n")
+  /** Development web server. */


👏 for comments

jsjoeio · 2021-11-18T18:44:08Z

ci/dev/watch.ts

    }
+
+    this.webServer = fork(path.join(this.rootPath, "out/node/entry.js"), process.argv.slice(2))


TIL about fork.

jsjoeio · 2021-11-18T18:45:43Z

ci/dev/watch.ts

+  /** Development web server. */
+  private webServer: ChildProcess | undefined
+
+  private reloadWebServer = (): void => {


❓ 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)

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

TIL! Thanks for the explainer!

jsjoeio · 2021-11-18T18:46:05Z

ci/dev/watch.ts


-    const vscode = cp.spawn("yarn", ["watch"], { cwd: this.vscodeSourcePath })
+  //#region Compilers


I like this pattern you've started introducing 👏 Does it have a name? Comment regions?

I wish I could take credit for it but it’s built into VS Code’s region collapsing.

jsjoeio · 2021-11-18T18:46:57Z

ci/dev/watch.ts

+    }
+
+    if (!this.hasVerboseLogging) {
+      console.log("\n[Watcher]", "Compiler logs will be minimal. Pass --log to show all output.")


Yes! Basically this hides it unless --log is passed when starting yarn watch? Fantastic DX improvement 🔥

jsjoeio · 2021-11-18T18:47:14Z

ci/dev/watch.ts

-      Watcher.log("killing tsc")
-      tsc.removeAllListeners()
-      tsc.kill()
+    this.cleanFiles()


this section looks a lot cleaner 🔥

jsjoeio · 2021-11-18T18:47:44Z

ci/dev/watch.ts

+        // 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!)")


🔥🔥 love the emojis in the logs :D

jsjoeio · 2021-11-18T18:49:04Z

package.json

@@ -52,6 +52,7 @@
    "@typescript-eslint/parser": "^5.0.0",
    "audit-ci": "^5.0.0",
    "codecov": "^3.8.3",
+    "del": "^6.0.0",


nit: should this be a devDependency?

Already is 😄

LOL does this mean my eyesight is going? Apologies!

jsjoeio · 2021-11-18T18:50:12Z

src/node/util.ts

@@ -25,10 +26,11 @@ const pattern = [
 ].join("|")
 const re = new RegExp(pattern, "g")

+export type OnLineCallback = (strippedLine: string, originalLine: string) => void


👏 appreciate the type extraction and naming to make it easier to read/maintain

- Clean up watcher behaviors.

jsjoeio

Woohoo!

jsjoeio · 2021-11-19T16:58:29Z

codecov/project — 65.44% (-0.60%) compared to dd29a82

It's less than a %. I think it's okay for us to ignore this

GirlBossRush added enhancement Some improvement that isn't a feature chore Related to maintenance or clean up labels Nov 15, 2021

GirlBossRush added this to the 4.0.0 milestone Nov 15, 2021

GirlBossRush requested a review from code-asher November 15, 2021 00:31

GirlBossRush self-assigned this Nov 15, 2021

GirlBossRush commented Nov 15, 2021

View reviewed changes

GirlBossRush force-pushed the watcher-improvements branch 3 times, most recently from 58f1c54 to e969ebb Compare November 15, 2021 01:06

GirlBossRush marked this pull request as ready for review November 18, 2021 18:13

GirlBossRush requested a review from a team as a code owner November 18, 2021 18:13

jsjoeio suggested changes Nov 18, 2021

View reviewed changes

Fix issues surrounding initial web server load.

f47abdf

- Clean up watcher behaviors.

jsjoeio approved these changes Nov 19, 2021

View reviewed changes

GirlBossRush force-pushed the watcher-improvements branch from 11fc940 to f47abdf Compare November 19, 2021 17:00

Merge branch 'main' into watcher-improvements

4b38316

GirlBossRush merged commit 3157a40 into main Nov 19, 2021

GirlBossRush deleted the watcher-improvements branch November 19, 2021 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watcher improvements #4509

Watcher improvements #4509

GirlBossRush commented Nov 15, 2021

GirlBossRush Nov 15, 2021

GirlBossRush Nov 15, 2021

jsjoeio Nov 18, 2021

github-actions bot commented Nov 15, 2021 •

edited

Loading

codecov bot commented Nov 15, 2021 •

edited

Loading

jsjoeio left a comment •

edited

Loading

jsjoeio Nov 18, 2021

GirlBossRush Nov 19, 2021

jsjoeio Nov 19, 2021

jsjoeio Nov 18, 2021

jsjoeio Nov 18, 2021

jsjoeio Nov 18, 2021

GirlBossRush Nov 19, 2021

jsjoeio Nov 19, 2021

jsjoeio Nov 18, 2021

GirlBossRush Nov 19, 2021

jsjoeio Nov 18, 2021

jsjoeio Nov 18, 2021

jsjoeio Nov 18, 2021

jsjoeio Nov 18, 2021

GirlBossRush Nov 19, 2021

jsjoeio Nov 19, 2021

jsjoeio Nov 18, 2021

jsjoeio left a comment

jsjoeio commented Nov 19, 2021

		}

		this.webServer = fork(path.join(this.rootPath, "out/node/entry.js"), process.argv.slice(2))


		const vscode = cp.spawn("yarn", ["watch"], { cwd: this.vscodeSourcePath })
		//#region Compilers

Watcher improvements #4509

Watcher improvements #4509

Conversation

GirlBossRush commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2021 • edited Loading

codecov bot commented Nov 15, 2021 • edited Loading

Codecov Report

jsjoeio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjoeio left a comment

Choose a reason for hiding this comment

jsjoeio commented Nov 19, 2021

github-actions bot commented Nov 15, 2021 •

edited

Loading

codecov bot commented Nov 15, 2021 •

edited

Loading

jsjoeio left a comment •

edited

Loading