-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(client): Add hot reload for client packages #49
base: edge-civictechto
Are you sure you want to change the base?
Conversation
{context: ['/api'], | ||
target: 'http://127.0.0.1:5000/', | ||
secure: false, | ||
changeOrigin: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this config, the client-admin
page can be accessed at localhost:3000
. And all the api request will be proxied to http://127.0.0.1:5000/
, where the server
container is hosted.
But this error is thrown from the client-admin
container:
[HPM] Error occurred while proxying request localhost:3000/api to http://127.0.0.1:5000/ [ECONNREFUSED]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it might be less obvious what's going on here to a future reader, perhaps leave the context for this in a comment?
Also, will this break things for future production builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to toggle this based on NODE_ENV? Although I'll admit that it's been helpful in the past to sometimes run a dev hot-reloading environment with NODE_ENV=production settings (minification, etc) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, this won't affect static build, right? Ok, if so, maybe just a comment explaining why it's there inline :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, i'll make sure to add some context! And yes, it's separate from the static builds. For now, I'm just trying to make it work.
@@ -22,4 +22,4 @@ RUN npm ci --production=false | |||
|
|||
COPY . . | |||
|
|||
CMD npm run build:prod | |||
CMD npm start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one consideration is that this affects both dev and production builds, so we wont be able to publish container images that we eventually host from using this Dockerfile.
But since we're not publishing container images, maybe we can defer this. I'd recommend adding a TODO comment above to acknowledge the minor "regression" in making this change :)
But otherwise, I love this idea!
CMD npm start | |
# Starts dev environment | |
# TODO: Figure out how to implement Dockerfile(s) that works for both prod images and dev container. | |
CMD npm start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i thought the prod deployments don't use this file as mentioned by the headline comment in this file:
# NOTE: This Dockerfile is not actually used by the docker-compose.yml.
# Instead, the file-server Dockerfile builds and serves these assets.
# But this file is still useful for development or deployments that do not use
# the docker compose configuration.
Maybe I understood it wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, you're right, that's worded a bit funny. Unless something has drastically changed (and I just did a cursory check that it hasn't) that's actually trying to warn ppl not to look for evidence or ability to serve the sub-apps in their dockerfiles. But the build process is still used, and the products of the build pulled into the file-server container (which ACTUALLY serves the content :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, i'll go with your suggested change
Nice! I'm realizing the bonus of submitting PRs upstream (at the same time) is that review there might allow us to benefit from the institutional knowledge of the build environment and its idiocyncrasies (often the product of lots of time spent learning and working around edge-cases) cc: @NewJerseyStyle (bc i think this means our branching strategy still needs consideration 🙃 ) |
Does this help? They have something for developers, may include hot reload? |
Fixes #47