Convert code-server http code to use Express #2238

code-asher · 2020-10-26T22:48:22Z

Story details: https://app.clubhouse.io/coder/story/1385

Couple items still remaining but existing code is ready to review:

I keep getting log directories created in the code-server directory over and over with new timestamps.
Need a way to register disposables for when the application is killed.

Closes #2128
Closes #1899

This file is going to get blasted in favor of Express.

The Express types were throwing errors but regenerating the lockfile resolved them.

It doesn't do anything yet.

src/node/cli.ts

src/node/entry.ts

src/node/heart.ts

wbobeirne

There's a lot in this PR that I don't have the context to parse, but I caught a couple of items. VSCode seems to fire up just fine for me though, no bugs that I found with some cursory usage.

The patchRouter is the only big unknown to me. I haven't worked with modifying the underlying implementation of Express to know enough about where to do that, but I'm pretty sure this ain't it.

src/node/proxy.ts

src/node/routes/vscode.ts

src/node/routes/index.ts

src/node/routes/proxy.ts

src/node/routes/update.ts

src/node/http.ts

src/node/proxy.ts

src/node/routes/index.ts

src/node/routes/static.ts

- Use accept header. - Match /login and /login/ exactly. - Match /static/ (trailing slash). - Use req.path. Same result but feels more accurate to me.

We now guarantee there is an address.

Co-authored-by: Teffen Ellis <TeffenEllis@users.noreply.github.com>

Opening the URL can fail if the user doesn't have something appropriate installed to handle it.

Makes typing much easier. Addresse's Will's last comment.

Unfortunately we can't use node-mocks-http to test a express.Router that has async routes. See eugef/node-mocks-http#225 router will just return undefined if the executing handler is async and so the test will have no way to wait for it to complete. Thus, we have to use supertest which starts an actual HTTP server in the background and uses a HTTP client to send requests.

Plugin API to add more applications to code-server

src/common/emitter.ts

src/node/wsRouter.ts

src/node/routes/index.ts

src/node/wsRouter.ts

nhooyr · 2020-11-10T20:24:18Z

src/node/update.ts

+            response.statusCode < 400 &&
+            response.headers.location
+          ) {
+            ++redirects


Yea if we can I'd much rather we use a library here. It's really error prone to write this all ourself. I'm sure we're probably missing a few HTTP gotchas too.

src/node/routes/index.ts

src/node/proxy.ts

That's the default so it's extra visual noise.

Hopefully is a bit easier to read.

Also add a check that the domain has a dot. This covers the localhost case as well, so remove that.

code-asher · 2020-11-11T00:55:18Z

I think all comments have been addressed except the following which I think we could tackle a bit later:

Store last update internally instead of to disk
Use a library to fetch the latest version from GitHub
Split or rename setDefaults

Lemme know if I missed something.

One problem though is that the error handler doesn't seem to work. It never gets called so we just get the default Express error page. I'm not sure if it has something to do with the Express 5 update (I didn't see anything relevant in the migration guide: https://expressjs.com/en/guide/migrating-5.html) or what.

nhooyr · 2020-11-12T15:39:54Z

One problem though is that the error handler doesn't seem to work. It never gets called so we just get the default Express error page. I'm not sure if it has something to do with the Express 5 update (I didn't see anything relevant in the migration guide: https://expressjs.com/en/guide/migrating-5.html) or what.

Let's make that a release blocking issue.

nhooyr

Approval after these last few concerns.

src/node/vscode.ts

nhooyr · 2020-11-12T15:51:40Z

src/node/vscode.ts

+    )
+
+    const message = await this.onMessage<ipc.OptionsMessage>(vscode, (message): message is ipc.OptionsMessage => {
+      // There can be parallel initializations so wait for the right ID.


How would a parallel init occur?

Loading the page in two separate tabs is the main way I think. If we had some sort of collaborative mode it would probably be a bigger problem then.

I guess in theory it could also happen if you refresh a page before it finishes loading to send off two almost-parallel requests although I don't think I've hit it that way before.

Loading the page in two separate tabs is the main way I think. If we had some sort of collaborative mode it would probably be a bigger problem then.

Shouldn't we prevent sending parallel init's at the same time?

Or actually, doesn't each websocket get it's own vscode process? Or is that only its own extension host?

There's a single VS Code process for all websockets and an extension host for each websocket. Each page also gets its own "management" websocket where we initialize all the services VS Code uses (like the file service, environment service, language initialization, and so on).

The services are shared too actually, come to think. Language stuff isn't though, and there are some other differences in the initialization like which folder to open up (depending on the query parameter for example).

src/node/vscode.ts

src/node/routes/domainProxy.ts

src/node/routes/vscode.ts

Turns out that while Typescript can't infer the callback return type from it, Typescript can do the opposite and infer it from the callback return type.

This will make the route more robust since it'll work under more than just the root.

nhooyr

🚀

src/node/vscode.ts

Also switch `once` to `on` since we `off` them later anyway so no point in making Node do it twice.

Feels like it parallels better with the other handlers.

I'm not sure why other builds are passing with this still in.

code-asher added 5 commits October 20, 2020 16:15

Move argument defaults into setDefaults

dcb303a

Move heart and AuthType out of http

2928d36

This file is going to get blasted in favor of Express.

Add Express as a dep and regenerate lockfile

6000e38

The Express types were throwing errors but regenerating the lockfile resolved them.

Move providers from app to routes

9f25cc6

Strip config file password from debug log

8e93e28

code-asher requested a review from nhooyr as a code owner October 26, 2020 22:48

code-asher changed the base branch from v3.6.1 to master October 26, 2020 22:48

code-asher added 4 commits October 26, 2020 17:56

Switch to Express

71dc5c7

It doesn't do anything yet.

Add file for global constants

4b6cbac

Convert routes to Express

112eda4

Make authentication work with sub-domain proxy

257d9a4

code-asher force-pushed the code-asher/ch1385 branch from 18fd7aa to 257d9a4 Compare October 26, 2020 22:56

code-asher requested a review from wbobeirne October 27, 2020 15:10