Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable custom sockjs pathname for hot reloading server. #7750

Merged
merged 4 commits into from
Jan 31, 2020

Conversation

heygrady
Copy link
Contributor

Fixes #7597
See also https://github.com/dmile/create-react-app/commit/d86b05e9d327b368f664f6b9846ecac9806bca70 and #7444 (comment)

Enables setting the sockPath option for the webpack-dev-server using a new WDS_SOCKET_PATH environment variable. This env controls the websocket connection pathname used by the hot reloading client/server.

Primary use-case would be starting a development server for multiple projects simultaneously. It's already possible to specify a PORT for the development server. When proxying multiple projects behind a single domain it is also necessary to customize the sockPath.

Why?

There are many valid reasons to develop locally with a custom hostname. It is also common in real-life productions apps to proxy to different apps based on pathname.

Imagine that we are running a local nginx proxy at local.example.com like so:

  • http://local.example.com/project-a -> localhost:3001
  • http://local.example.com/project-b -> localhost:3002
# start two projects with different ports and different sock paths
$ PORT=3001 WDS_SOCKET_PATH=/sockjs-node/project-a yarn start
$ PORT=3002 WDS_SOCKET_PATH=/sockjs-node/project-b yarn start

Of course we also need to proxy the sockjs pathnames with nginx as well.

This rig enables an app to be developed similarly to how it will appear in production.

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, it looks good. Only a few minor suggestions from me!

packages/react-scripts/config/webpackDevServer.config.js Outdated Show resolved Hide resolved
docusaurus/docs/advanced-configuration.md Outdated Show resolved Hide resolved
@mrmckeb mrmckeb self-assigned this Sep 30, 2019
@mrmckeb mrmckeb added this to the 3.2 milestone Sep 30, 2019
@robertvansteen
Copy link
Contributor

Perhaps we should add support for configuring the other values (port, hostname) here as well? That way if you serve the app over a different hostname (e.g. myapp.localhost) it can still connect with localhost:3000.

@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 1, 2019

@heygrady I like @rovansteen's suggestion. Do you think you could get that in here?

Something happened to the CI, sorry about that. I know it's working better today, but still needs love.

@robertvansteen
Copy link
Contributor

This looks good to me if we can get the build green. Are the issues with the CI resolved now?

@heygrady
Copy link
Contributor Author

rebased and force pushed

@heygrady
Copy link
Contributor Author

Oh, needs to have more recent version of webpack-dev-server.

@heygrady
Copy link
Contributor Author

heygrady commented Oct 25, 2019

The other options (sockPort, sockHost) that @rovansteen wanted aren't available in version 3.2.0 of webpack-dev-server that react-scripts is using. They were added in version 3.4.0

I went ahead and upgraded to version 3.9.0.

@heygrady
Copy link
Contributor Author

The kitchen sink tests are failing because of a cryptic error, TypeError: Cannot read property '_cookieJar' of null. This may be related to upgrade of webpack dev server (but it's not clear how). There are some hints that this is a JSDOM issue (jestjs/jest#8613). It's not easy to run the kitchen sink tests locally to determine what might fix it.

@heygrady heygrady force-pushed the sock-path branch 2 times, most recently from dc401b3 to eb7bef9 Compare October 29, 2019 18:03
@heygrady
Copy link
Contributor Author

@mrmckeb I'm curious about next steps. I don't suspect that the CI kitchensink failures are directly related to this PR. I run a fork where I've had the upgraded webpack-dev-server for quite a while and I don't have any issues.

@matejstrasek
Copy link

Would be wonderful to have this merged. We are running multiple projects simultaneously and this PR would make life much easier.

@twschiller
Copy link

@matejstrasek I put together a gitpkg of this PR at https://github.com/twschiller/create-react-app/tree/react-scripts-v3.2.0-gitpkg. Add it to your packages.json like so:

"react-scripts": "git+https://github.com/twschiller/create-react-app.git#react-scripts-v3.2.0-gitpkg",

@heygrady
Copy link
Contributor Author

heygrady commented Nov 18, 2019

Related: #7988

I experimented with rebasing this branch on top of #7988 and that worked great. This branch is block by that branch.

@heygrady heygrady force-pushed the sock-path branch 2 times, most recently from 8f7e13d to 6f20cfd Compare November 18, 2019 22:00
@heygrady
Copy link
Contributor Author

Rebased. All tests should be passing now.

@ianschmitz ianschmitz modified the milestones: 3.3, 3.4 Dec 5, 2019
@shenst1
Copy link

shenst1 commented Jan 15, 2020

Was wondering if this is still going to be pushed through now that it has been approved? Looks like the milestone has passed

@runofthemillgeek
Copy link

Would love to see this merged. We ran into this while we were migrating our legacy codebase incrementally over to React and we're including the React bundle in our main application while running two distinct dev servers. In this scenario, we need to override the defaults to get HMR to work.

@heygrady any idea why builds are failing? I'm unable to see the logs on Azure Pipelines.

@andriijas andriijas merged commit 822422c into facebook:master Jan 31, 2020
@andriijas
Copy link
Contributor

Thanks for your patience everyone!

Please note that env var configuration might be changed to config file in the future.

@lock lock bot locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow hot reload sockPath to be changed