Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Watcher improvements #4509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
GirlBossRush merged 2 commits into main from watcher-improvements
Nov 19, 2021
Merged

Watcher improvements #4509

GirlBossRush merged 2 commits into main from watcher-improvements
Nov 19, 2021

Conversation

Copy link
Contributor

@GirlBossRush GirlBossRush commented Nov 15, 2021

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.

jsjoeio reacted with thumbs up emoji
@GirlBossRush GirlBossRush added enhancement Some improvement that isn't a feature chore Related to maintenance or clean up labels Nov 15, 2021
@GirlBossRush GirlBossRush added this to the 4.0.0 milestone Nov 15, 2021
import { CompilationStats, onLine, OnLineCallback, VSCodeCompileStatus } from "../../src/node/util"

interface DevelopmentCompilers {
[key: string]: ChildProcess | undefined
Copy link
Contributor Author

@GirlBossRush GirlBossRush Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jsjoeio reacted with thumbs up emoji
}

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

@GirlBossRush GirlBossRush Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jsjoeio reacted with thumbs up emoji jsjoeio reacted with hooray emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

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

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

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

codecov bot commented Nov 15, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph 

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

@GirlBossRush GirlBossRush marked this pull request as ready for review November 18, 2021 18:13
@GirlBossRush GirlBossRush requested a review from a team as a code owner November 18, 2021 18:13
Copy link
Contributor

@jsjoeio jsjoeio left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@GirlBossRush GirlBossRush Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh got it! Disregard 👍

process.stdout.write(message)
if (!skipNewline) {
process.stdout.write("\n")
/** Development web server. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 for comments

ci/dev/watch.ts Outdated
this.webServer.kill()
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about fork.

/** Development web server. */
private webServer: ChildProcess | undefined

private reloadWebServer = (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@GirlBossRush GirlBossRush Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jsjoeio reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! Thanks for the explainer!

//#endregion

constvscode=cp.spawn("yarn",["watch"],{cwd: this.vscodeSourcePath})
//#region Compilers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@GirlBossRush GirlBossRush Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jsjoeio reacted with thumbs up emoji jsjoeio reacted with eyes emoji
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🔥 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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be a devDependency?

Copy link
Contributor Author

@GirlBossRush GirlBossRush Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsjoeio reacted with laugh emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL does this mean my eyesight is going? Apologies!

].join("|")
const re = new RegExp(pattern, "g")

export type OnLineCallback = (strippedLine: string, originalLine: string) => void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo!

Copy link
Contributor

jsjoeio commented Nov 19, 2021

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 GirlBossRush deleted the watcher-improvements branch November 19, 2021 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@code-asher code-asher Awaiting requested review from code-asher

1 more reviewer

@jsjoeio jsjoeio jsjoeio approved these changes

Reviewers whose approvals may not affect merge requirements
Labels
chore Related to maintenance or clean up enhancement Some improvement that isn't a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /