dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.68k stars 1.06k forks source link

Feature request: Support static WebSockets `wss://` and `ws://` port(s) via env var `DOTNET_WATCH_AUTO_RELOAD_WS_ENDPOINT` in `dotnet watch` #31735

Open todthomson opened 1 year ago

todthomson commented 1 year ago

Disclaimer: I'm new here, so please let me know if I'm doing anything at all wrong and I'll fix it. šŸ˜„

Is your feature request related to a problem? Please describe.

Each time dotnet watch restarts (manually, or in response to a rude edit, etc) random free ports (via port :0) are chosen for the ws:// and wss:// WebSockets connection,

I can appreciate why this feature defaults to "a random free port" (i.e. you want to make the feature "just work", rather than trying to bind to a user-defined static port), but this has some negative effects of the developer experience, i.e. that:

  1. The WebSockets connection(s) cannot be automatically resumed (reconnected to the existing browser tabs) by aspnetcore-browser-refresh.js, as the ports have changed.
  2. Practically, this means that you need to F5 the browser window, to reload the page, so that aspnetcore-browser-refresh.js will pick up the new WS ports.
  3. Thus, no more "hot" or even "live" reload for you, until you F5.

This issue has been raised twice before:

  1. https://github.com/dotnet/aspnetcore/issues/39608
  2. https://github.com/dotnet/aspnetcore/issues/33823

Describe the solution you'd like

Setting the environment variable as follows would allow developers to opt-in to static wss:// and ws:// WebSockets ports to be specified, e.g. like:

DOTNET_WATCH_AUTO_RELOAD_WS_ENDPOINT=https://localhost:43399,http://localhost:8099

I'd like to propose this as a minor change to .NET SDK 7.x (as this can be shipped as an additive non-breaking change).

I have a PR here which shows an example implementation.

See: https://github.com/dotnet/sdk/pull/31575/files for the code.

Additional context

The original (implemented and closed) browser live reload feature (as envisaged by @DamianEdwards) even covered the scenario we're talking about here, but it doesn't work because of the random port selection on restart.

The JS block should treat any disconnection of the WebSocket as indication it needs to perform the browser refresh. In the case of dotnet watch, this should only occur after it has been determined that the application server has successfully restarted after the project has been rebuilt and restarted. -- Damian Edwards

See: https://github.com/dotnet/aspnetcore/issues/23412#issuecomment-655205989 for more details.

Challenges I am having

I'm currently struggling to work out how to build .NET SDK 7.x from source. I have it building from source as a patch to .NET SDK 8, but my example app doesn't work currently when ported to .NET 8, so it's making it hard to test the patch locally. Please point me to the correct branch and build "how to" and I'm happy to rebase this from the 7.x branch.

Update: I've found branches release/7.0.2xx, release/7.0.3xx, and release/7.0.4xx, which all compile to produce v7 of the SDK fine, but apologies, I'm not sure if I'm supposed to raise PRs against one/many of those, or some other upstream 7.x branch?

We'd also need to adjust the behaviour of aspnetcore-browser-refresh.js slightly so that it will e.g. window.location.reload() when it detects a subsequent drop and reconnection of the WebSocket(s), but at this stage, I'm just focussed on seeing if this change to static ports is one that's amenable to the team.

Thanks, and I appreciate you taking the time to consider my proposal. šŸ„°

Cheers,

Tod.

cc: @tmat @arkalyanms (as code owners of Area-Watch)

tmat commented 1 year ago

@tlmii

tmat commented 1 year ago

@todthomson Shouldn't the port number be an optional argument to dotnet watch rather than env variable? Is there a benefit of env variable?

DamianEdwards commented 1 year ago

If we updated aspnetcore-browser-refresh.js to auto-reload the window when the websocket is disconnected, wouldn't there be no requirement for the static port anymore? The page would reload from the running app, which has the refresh middleware injected with the correct port number by the host (dotnet-watch).

todthomson commented 1 year ago

@todthomson Shouldn't the port number be an optional argument to dotnet watch rather than env variable? Is there a benefit of env variable?

Iā€™m proposing DOTNET_WATCH_AUTO_RELOAD_WS_ENDPOINT (or one might prefer to call it DOTNET_WATCH_HOT_RELOAD_WS_URLS, etc) just to:

  1. try to keep the feature request as simple as possible (in the hopes we can get it into SDK 7 as a minor, or SDK 8), and
  2. as a similar as possible to the old feature in ASPNET Core ASPNETCORE_AUTO_RELOAD_WS_ENDPOINT as per the first issue above.

That being said, I can certainly imagine a more complete feature working roughly as follows:

  1. Pass command line arguments to dotnet watch e.g. dotnet watch --hot-reload-wss-port 43399 --hot-reload-ws-port 8099 or even dotnet watch --hot-reload-urls https://localhost:43399,http://localhost:8099.

  2. Set environment variable as above.

  3. Setting the ports for a project in appsettings.json similarly to applicationUrl, e.g.

ā€œYourAppName": {
      "launchBrowser": false,
      "hotReloadEnabled": true,
      "hotReloadProfile": "aspnetcore",
      "hotReloadUrls": "https://localhost:43399,http://localhost:8099"
      }

And, Iā€™d presume they would apply into order of priority above i.e.

  1. Command line has the highest priority, as it allows the user to override environment variables or default project config.

  2. Environment variable (if not specified on command line) would override the project defaults, allowing the user to reconfigure their environment.

  3. If neither command line or environment variables are configured, then allow the project to define well-known wss:// and ws:// ports.

I canā€™t imagine having all of the above would be a bad thing, itā€™s just more code and more testing is all, and the environment variable would be good enough (for me) for now. :)

todthomson commented 1 year ago

If we updated aspnetcore-browser-refresh.js to auto-reload the window when the websocket is disconnected, wouldn't there be no requirement for the static port anymore? The page would reload from the running app, which has the refresh middleware injected with the correct port number by the host (dotnet-watch).

I might be making an incorrect assumption here, so if we can make this work with only a change to aspnetcore-browser-refresh.js, then that would be awesome and very much satisfy my current needsā€¦ i.e. as it stands now, each time dotnet watch detects a ā€œrude editā€ (and therefore it restarts) the ports change to two other random free ports, which results in hot reload / browser refresh stopping working (until you notice that) and F5 the browser (to pick up the new WS ports).

My assumption (perhaps incorrectly) is that when dotnet watch detects the ā€œrude editā€ and decides it needs to restart, that will disconnect the WS, triggering the close event, and then when it restarts on new ports, the browser will never see the corresponding open event on re-connection of the WS, as for WS to work, Iā€™m assuming both ends have to agree on ports in advance, and there is no side-channel on which to re-negotiate a new set of ports.

As I said, would be very glad to be wrong on that assumption, as it would reduct the size of the change to only aspnetcore-browser-refresh.js, which I could (I presume) easily copy into my app and make that change to in the short term.

Iā€™m new to WS and working into the bowls of the .NET SDK, so very happy to take direction from anyone(s) who has more experience in these areas. :) Cheers.

DamianEdwards commented 1 year ago

Right, I'm thinking that the js could simply be updated to do a browser-side refresh (automate F5 if you will) when the websocket is closed on the browser side (assuming it's immediate) to the app address (which is a separate URL and port to the websocket). The refresh will load the "new" page (from the restarted app) and inline render the js which now was the new port dotnet watch is listening on.

todthomson commented 1 year ago

Presuming it doesnā€™t result in a 404 because the web server is restarting, and it doesnā€™t leave everyone elseā€™s browsers with blank tabs, I am all for it. :)

Iā€™ll experiment and report back asap.

DamianEdwards commented 1 year ago

Yep those are totally fair concerns. One of the hardest part of these kinds of tooling experiences is coordinating all the moving pieces to avoid those dreaded browser error screens. I admit I'm surprised dotnet watch is restarting the websocket server after each app restart, as ultimately that's what causing the browser to lose the connection and thus not get the signal to refresh. I'm trying to cast my mind back but I can't remember why that would be the case. Perhaps that can be revisited.

todthomson commented 1 year ago

Yep those are totally fair concerns. One of the hardest part of these kinds of tooling experiences is coordinating all the moving pieces to avoid those dreaded browser error screens. I admit I'm surprised dotnet watch is restarting the websocket server after each app restart, as ultimately that's what causing the browser to lose the connection and thus not get the signal to refresh. I'm trying to cast my mind back but I can't remember why that would be the case. Perhaps that can be revisited.

Appreciate that mate. I can't really say for sure that dotnet watch is restarting, only that when a "rude edit" happens, and dotnet watch chooses to reboot dotnet run, then WebSockets connections end up with new ports, which as you say means the connection is (sadly) lost for good (until the F5 grabs the new port configuration from the server).

Iā€™ll experiment and report back asap. Thanks.