Add proxy support #32

rusty-key · 2024-09-05T11:08:23Z

This PR adds basic support for proxying by putting node http server in front of esbuild

jchavarri

Awesome!

jchavarri · 2024-09-05T14:05:50Z

commands/reshowcase

-  const port = getPort();
-  const durationLabel = "[Reshowcase] Watch and serve started. Duration";
-  console.time(durationLabel);
+  const clientPort = getPort();


clientPort is a bit weird naming as a port is always on the server. What about userFacingPort? Or userPort. Or just port as before (the esbuild one is an internal detail really)

jchavarri · 2024-09-05T14:06:08Z

commands/reshowcase

-    .then((_serveResult) => {
-      console.timeEnd(durationLabel);
-      console.error("[Reshowcase] Watch mode started on port:", port);
+    .then((s) => {


I'd name s -> esbuildServer

rusty-key · 2024-09-05T14:08:38Z

I also realized that we need socket support for this. I'll add it before merging

jchavarri · 2024-09-05T14:27:04Z

commands/reshowcase

@@ -225,11 +226,55 @@ if (isBuild) {
      process.exit(1);
    })
    .then((ctx) => {
-      return ctx.serve({ port: port, servedir: outputPath });
+      return ctx.serve({ servedir: outputPath })


kind of minor: as esbuild will pick up a random port if none is defined, and the esbuild server is created before the proxy one, maybe we could set esbuild port as port + 1?

To ensure that esbuild doesn't accidentally take useful port?

Yeah to make sure that esbuild picks port 3000 and user passes 3000 as well from command line. I think it's very unlikely though if esbuild picks the first available port... feel free to ignore.

rusty-key

Logging should be improved, and script itself should be more robust, but we can improve it step-by-step

rusty-key · 2024-09-07T14:03:02Z

commands/reshowcase

+              ...options.headers,
+              ...(rewrite.changeOrigin ? { host: `${options.host}:${options.port}` } : {})
+            };
+            console.info(`forwarding ${req.url} to ${options.host}:${options.port}`);


I added these logs, but maybe it's too verbose. Probably should hide behind some flag

Maybe we should add [Reshowcase] as the other logs?

rusty-key · 2024-09-07T14:03:31Z

commands/reshowcase

+        proxyReq.on("error", err => {
+          console.error("Proxy request error:", err);
+          res.writeHead(500, { "Content-Type": "text/plain" });
+          res.end("Internal Server Error");
+        });


Probably should show less details here as well

I think in this case we want to know as much as possible. Would definitely keep them (the same comment about [Reshowcase] prefix applies).

jchavarri

For some reason, live reload doesn't work for me. I have tried this branch, esbuild branch, working locally, on nspawn, with async/await... nothing.

It's strange because i use the same mechanism for react-alicante workshop and over there it works great.

jchavarri · 2024-09-09T06:35:48Z

commands/reshowcase

+              ...options.headers,
+              ...(rewrite.changeOrigin ? { host: `${options.host}:${options.port}` } : {})
+            };
+            console.info(`forwarding ${req.url} to ${options.host}:${options.port}`);


Maybe we should add [Reshowcase] as the other logs?

jchavarri · 2024-09-09T06:36:14Z

commands/reshowcase

+        if (rewrite) {
+          if (rewrite.socketPath) {
+            options.socketPath = rewrite.socketPath;
+            console.info(`forwarding ${req.url} to ${options.socketPath}`)


Maybe we should add [Reshowcase] as the other logs?

jchavarri · 2024-09-09T06:37:00Z

commands/reshowcase

+        proxyReq.on("error", err => {
+          console.error("Proxy request error:", err);
+          res.writeHead(500, { "Content-Type": "text/plain" });
+          res.end("Internal Server Error");
+        });


I think in this case we want to know as much as possible. Would definitely keep them (the same comment about [Reshowcase] prefix applies).

jchavarri · 2024-09-09T06:38:38Z

commands/reshowcase

+
+      server.listen(port, error => {
+        if (error) {
+          return console.error(error)


[Reshowcase] prefix.

jchavarri · 2024-09-09T06:39:06Z

commands/reshowcase

+          return console.error(error)
+        }
+
+        console.log(`[Reshowcase] Server listening on port ${port}`)


Should we mention something about "watch mode"? The previous message was Watch and serve started.. Also the timings were useful (imo).

Also, would be useful to print full URL so that one can cmd-click from the terminal and open the browser tab directly, if desired.

rusty-key · 2024-09-09T10:53:04Z

For some reason, live reload doesn't work for me. I have tried this branch, esbuild branch, working locally, on nspawn, with async/await... nothing.

After looking at it, I am not sure I understand at all how it supposed to work. Script is completely dependent on dune, and it fully restarts after every build. So, is watch doing anything? We discard it as soon as rebuild happens.

jchavarri · 2024-09-09T12:36:57Z

Ah, great point! That seems to be the issue. If I run in separate terminals:

dune build @example -w
./commands/reshowcase start --entry=_build/default/example/example/example/Demo.js --output=_build/default/build

Then it works fine. I think this means we have to keep the esbuild command running, and we have to move away from the "Dune does everything" approach we were following.

Let me know if I can help somehow!

jchavarri · 2024-09-09T12:50:41Z

I added a comment for the live reload in the upstream PR because it's probably out of the scope in this one.

rusty-key · 2024-09-09T13:18:52Z

I'll merge this to upstream PR and continue with fixes there to avoid conflicts

rusty-key force-pushed the esbuild-proxy branch 2 times, most recently from 4f96371 to 4bc559e Compare September 5, 2024 13:54

try out proxy possibility

a13cb07

rusty-key force-pushed the esbuild-proxy branch from 4bc559e to a13cb07 Compare September 5, 2024 13:56

rusty-key requested review from jchavarri and denis-ok September 5, 2024 13:58

rusty-key marked this pull request as ready for review September 5, 2024 13:58

jchavarri approved these changes Sep 5, 2024

View reviewed changes

rusty-key added 2 commits September 5, 2024 16:21

add socket support

9ff7335

clarify variable names

215425a

jchavarri reviewed Sep 5, 2024

View reviewed changes

rusty-key added 2 commits September 5, 2024 17:04

do not omit headers

b27ba4c

ensure that esbuild doesn’t take server’s port

9e3debe

rusty-key commented Sep 7, 2024

View reviewed changes

rusty-key requested a review from jchavarri September 7, 2024 14:04

jchavarri reviewed Sep 9, 2024

View reviewed changes

rusty-key added 2 commits September 9, 2024 11:26

add [reshowcase] labels to logs

6121837

add timings to logs

92874ad

jchavarri mentioned this pull request Sep 9, 2024

Switch from webpack to esbuild #19

Merged

add more labels to logs

f638cb2

rusty-key merged commit 4d6625e into esbuild Sep 9, 2024

rusty-key deleted the esbuild-proxy branch September 9, 2024 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proxy support #32

Add proxy support #32

rusty-key commented Sep 5, 2024 •

edited

Loading

jchavarri left a comment

jchavarri Sep 5, 2024

jchavarri Sep 5, 2024

rusty-key commented Sep 5, 2024

jchavarri Sep 5, 2024

rusty-key Sep 5, 2024

jchavarri Sep 5, 2024

rusty-key left a comment

rusty-key Sep 7, 2024

jchavarri Sep 9, 2024

rusty-key Sep 7, 2024

jchavarri Sep 9, 2024

jchavarri left a comment

jchavarri Sep 9, 2024

jchavarri Sep 9, 2024

jchavarri Sep 9, 2024

jchavarri Sep 9, 2024

jchavarri Sep 9, 2024

jchavarri Sep 9, 2024

rusty-key commented Sep 9, 2024

jchavarri commented Sep 9, 2024

jchavarri commented Sep 9, 2024

rusty-key commented Sep 9, 2024

Add proxy support #32

Add proxy support #32

Conversation

rusty-key commented Sep 5, 2024 • edited Loading

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusty-key commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusty-key left a 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

jchavarri left a 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

rusty-key commented Sep 9, 2024

jchavarri commented Sep 9, 2024

jchavarri commented Sep 9, 2024

rusty-key commented Sep 9, 2024

rusty-key commented Sep 5, 2024 •

edited

Loading