Conversation
|
✨ Coder.com for PR #4499 deployed! It will be updated on every commit.
|
code-asher
left a comment
There was a problem hiding this comment.
Thank you for doing this! I know it sucks to maintain legacy behavior. Once plugins are removed we can revert this behavior as well if we want. 😄
| for (const routePrefix of ["/", "/vscode"]) { | ||
| app.router.use(routePrefix, vsServerRouteHandler.router) | ||
| app.wsRouter.use(routePrefix, vsServerRouteHandler.wsRouter) |
| router.all("*", ensureAuthenticated, (req, res, next) => { | ||
| req.on("error", (error: any) => { | ||
| private $proxyRequest: express.Handler = async (req, res, next) => { | ||
| // We allow certain errors to propagate so that other routers may handle requests |
There was a problem hiding this comment.
👏
❓ why start the name with $? is that some sort of special pattern?
There was a problem hiding this comment.
It’s something of a convention I’ve seen help with readability, similarly to prefixing private methods with _. Prefixing a method like $proxyRequest helps indicate a unique purpose compared to a more lengthy proxyRequestHandler
|
|
||
| try { | ||
| this._codeServerMain = await createVSServer(null, { | ||
| connectionToken: "0000", |
There was a problem hiding this comment.
❓ What's the connectionToken used for again?
There was a problem hiding this comment.
Connection token is used by VS Code as a basic form of client-side security. At the moment we’ve disabled its usage, but I think it’s worth looking at again as upstream matures the feature.
|
One observation (this is not from this PR since I remember seeing this before) but I get a 404 when VS Code is still loading rather than the "VS Code may still be compiling" error (running
|
b80725d to
e56d970
Compare
Codecov Report
@@ Coverage Diff @@
## main #4499 +/- ##
==========================================
- Coverage 66.46% 65.93% -0.53%
==========================================
Files 30 30
Lines 1625 1638 +13
Branches 330 330
==========================================
Hits 1080 1080
- Misses 463 475 +12
- Partials 82 83 +1
Continue to review full report at Codecov.
|
* Fix : recreate the termux guide to adapt the recent changes Termux nodejs-lts changed from v14 to v16 and there are many issues people are facing such as with argon2. Hence I recommend changing it to this install process which is comparably better and has one less issue :^) I've also added some extra things such as installing GO and Python, idk about the TOC tree but this is pretty much it. * yarn-fmt and minor typos #4472 (comment) * Fix : replace unnecessary steps to be linked to a guide * Change from private gist to a section in Extra * Remove reference to non-existent step * ready to merge! Co-authored-by: Joe Previte <jjprevite@gmail.com>
Fixes #4481