fix(remix-dev): re-use `REMIX_DEV_SERVER_WS_PORT` when reloading config #3206

mcansh · 2022-05-16T16:29:40Z

closes REM-1091

Signed-off-by: Logan McAnsh logan@mcan.sh

Docs
Tests

Testing Strategy:

I opened up a new playground (npm run playground:new) with a basic node server running on port 8002 and then created a few new files to trigger the config reload while logging the current REMIX_DEV_SERVER_WS_PORT.

tessellator · 2022-05-16T17:11:31Z

I think this can still get you in trouble wrt #296. If you set REMIX_DEV_SERVER_WS_PORT to the same value in two different projects, you would lose the advantage of using getPort. I will test to verify. I provided a similar solution in #3202 that will work in two projects simultaneously.

tessellator · 2022-05-16T17:25:05Z

Confirmed, but on second thought this behavior might be desirable. This PR makes REMIX_DEV_SERVER_WS_PORT explicit and will fail fast if the port is already taken, whereas the other PR is a "best effort" and will rewrite REMIX_DEV_SERVER_WS_PORT if the port is already taken.

(Note that you still get the advantage of getPort if REMIX_DEV_SERVER_WS_PORT is not explicitly set.)

kentcdodds

Thanks! What do you think about this?

packages/remix-dev/config.ts

closes REM-1091 closes #2958 Signed-off-by: Logan McAnsh <logan@mcan.sh>

tessellator · 2022-05-17T00:01:21Z

packages/remix-dev/config.ts

+  if (process.env.REMIX_DEV_SERVER_WS_PORT) {
+    try {
+      devServerPort = Number(process.env.REMIX_DEV_SERVER_WS_PORT);
+    } catch (error: unknown) {


I'm unsure if Number ever throws an error. In most cases it will return NaN instead.

In this part of the code, it would mean that someone explicitly set the env var to an invalid value. If it wasn't set explicitly, then it would either 1) fall through to getPort on the initial load, or 2) have been set to the output of getPort on a previous load.

Is getPort as default the right thing to do if someone explicitly sets the env var to an invalid value? (Genuinely curious as to the philosophy of the Remix team.)

yup, you're right on the NaN, Mondays amirite 😅. i'll wait for @kentcdodds' thoughts on the default behavior, i could see throwing an error as the desired behavior

Awesome. :) Thanks so much for working on this. I'm excited to see LiveReload working again!

Signed-off-by: Logan McAnsh <logan@mcan.sh>

kentcdodds

Another thought

packages/remix-dev/config.ts

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

kentcdodds

Sweet! Thanks 👏👏

packages/remix-dev/config.ts

kentcdodds

Whoops 😅 Ok, should be good now!

packages/remix-dev/config.ts

kentcdodds

I really shouldn't use our CI as my development environment 😅

github-actions · 2022-05-17T19:12:58Z

🤖 Hello there,

We just published version v1.5.0-pre.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2022-05-18T20:05:55Z

🤖 Hello there,

We just published version v1.5.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

EkaanshArora · 2022-06-03T13:16:50Z

Hi team,

This breaks out of the box hot-reload with some of the templates (eg: just the basics + express) as the used port is different from the expected 8002 default port for these configurations. (#3237)

This is because the new code gets a random available port if the environment variable isn't set - it's undefined for the templates as remix.config.js does not include a devServerPort.

Should the templates be updated to have "devServerPort": 8002 in the remix.config.js?

kentcdodds · 2022-06-07T21:18:42Z

@EkaanshArora, thanks. @mcansh is going to look into this.

danditomaso · 2022-10-28T02:44:45Z

Any updates on this issue?

remix-cla-bot bot added the CLA Signed label May 16, 2022

MichaelDeBoey added the package:dev label May 16, 2022

MichaelDeBoey changed the title ~~fix(remix-dev): re-use REMIX_DEV_SERVER_WS_PORT when reloading config~~ fix(remix-dev): re-use REMIX_DEV_SERVER_WS_PORT when reloading config May 16, 2022

MichaelDeBoey approved these changes May 16, 2022

View reviewed changes

jacob-ebey approved these changes May 16, 2022

View reviewed changes

kentcdodds requested changes May 16, 2022

View reviewed changes

packages/remix-dev/config.ts Outdated Show resolved Hide resolved

mcansh force-pushed the logan-rem-1091-fix-livereload branch 2 times, most recently from a8aec5f to 97d385e Compare May 16, 2022 22:25

tessellator mentioned this pull request May 16, 2022

fix(remix-dev): ensure devServerPort config value is the same across reloads #3202

Closed

1 task

fix(remix-dev): re-use REMIX_DEV_SERVER_WS_PORT when reloading config

9b9ad25

closes REM-1091 closes #2958 Signed-off-by: Logan McAnsh <logan@mcan.sh>

mcansh force-pushed the logan-rem-1091-fix-livereload branch from 97d385e to 9b9ad25 Compare May 16, 2022 23:41

tessellator reviewed May 17, 2022

View reviewed changes

chore(remix-dev): NaN fallback

a600343

Signed-off-by: Logan McAnsh <logan@mcan.sh>

kentcdodds requested changes May 17, 2022

View reviewed changes

packages/remix-dev/config.ts Outdated Show resolved Hide resolved

Update packages/remix-dev/config.ts

0ea0631

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

kentcdodds approved these changes May 17, 2022

View reviewed changes

kentcdodds reviewed May 17, 2022

View reviewed changes

packages/remix-dev/config.ts Outdated Show resolved Hide resolved

Update packages/remix-dev/config.ts

14963ea

kentcdodds approved these changes May 17, 2022

View reviewed changes

kentcdodds reviewed May 17, 2022

View reviewed changes

packages/remix-dev/config.ts Outdated Show resolved Hide resolved

Update packages/remix-dev/config.ts

f338584

kentcdodds approved these changes May 17, 2022

View reviewed changes

kentcdodds merged commit 1ab9745 into dev May 17, 2022

kentcdodds deleted the logan-rem-1091-fix-livereload branch May 17, 2022 15:37

MichaelDeBoey mentioned this pull request May 17, 2022

Remix dev asset server web socket error #2958

Closed

eric-burel mentioned this pull request May 18, 2022

Remix "Eurodance Stack" aka "Enka Stack" aka Vulcan Remix VulcanJS/vulcan-npm#117

Open

41 tasks

EkaanshArora mentioned this pull request Jun 3, 2022

devServerPort not set on fresh install with "Just the basics" & "Express Server" target #3237

Closed

mcansh mentioned this pull request Jun 20, 2022

Live Reload stops working soon after initial page refresh remix-run/indie-stack#73

Closed

donavon mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#17

Open

snyk-bot mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#19

Open

donavon mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#20

Open

machour mentioned this pull request Jan 22, 2023

Live reloading server breaks when file deleted #3130

Closed

This was referenced Nov 28, 2023

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#34

Open

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(remix-dev): re-use `REMIX_DEV_SERVER_WS_PORT` when reloading config #3206

fix(remix-dev): re-use `REMIX_DEV_SERVER_WS_PORT` when reloading config #3206

mcansh commented May 16, 2022 •

edited

Loading

tessellator commented May 16, 2022

tessellator commented May 16, 2022 •

edited

Loading

kentcdodds left a comment

tessellator May 17, 2022

mcansh May 17, 2022

tessellator May 17, 2022

kentcdodds left a comment

kentcdodds left a comment

kentcdodds left a comment

kentcdodds left a comment

github-actions bot commented May 17, 2022

github-actions bot commented May 18, 2022

EkaanshArora commented Jun 3, 2022

kentcdodds commented Jun 7, 2022

danditomaso commented Oct 28, 2022

fix(remix-dev): re-use REMIX_DEV_SERVER_WS_PORT when reloading config #3206

fix(remix-dev): re-use REMIX_DEV_SERVER_WS_PORT when reloading config #3206

Conversation

mcansh commented May 16, 2022 • edited Loading

tessellator commented May 16, 2022

tessellator commented May 16, 2022 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

tessellator May 17, 2022

Choose a reason for hiding this comment

mcansh May 17, 2022

Choose a reason for hiding this comment

tessellator May 17, 2022

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

github-actions bot commented May 17, 2022

github-actions bot commented May 18, 2022

EkaanshArora commented Jun 3, 2022

kentcdodds commented Jun 7, 2022

danditomaso commented Oct 28, 2022

fix(remix-dev): re-use `REMIX_DEV_SERVER_WS_PORT` when reloading config #3206

fix(remix-dev): re-use `REMIX_DEV_SERVER_WS_PORT` when reloading config #3206

mcansh commented May 16, 2022 •

edited

Loading

tessellator commented May 16, 2022 •

edited

Loading