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.

Sign up for GitHub

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

Jump to bottom

feat(client): Add hot reload for client packages #49

Draft
wants to merge 1 commit into
base: edge-civictechto
Choose a base branch
from
Draft

feat(client): Add hot reload for client packages #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client-admin/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ RUN npm ci --production=false

COPY . .

CMD npm run build:prod
CMD npm start
Copy link
Member

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!

Suggested change
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

Copy link
Collaborator Author

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?

Copy link
Member

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 :) )

https://github.com/compdemocracy/polis/blob/c60752b2007578073ef788069fe7ba06fbe89c3f/file-server/Dockerfile#L102-L104

Copy link
Collaborator Author

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

9 changes: 9 additions & 0 deletions client-admin/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ module.exports = (env, options) => {
},
},
**/

port: 3000,
proxy: [
{context: ['/api'],
target: 'http://127.0.0.1:5000/',
secure: false,
changeOrigin: true,
Copy link
Collaborator Author

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]

Copy link
Member

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?

Copy link
Member

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) 🤔

Copy link
Member

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 :)

Copy link
Collaborator Author

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.

}
]
},
plugins: [
new CopyPlugin({
Expand Down
18 changes: 18 additions & 0 deletions docker-compose.dev.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,21 @@ services:
# live-code-reloaded ports
# * leave the server pointing to the static builds, but have separate ports you can hit for the live-code
# reloading (making sure to document the process)

client-admin:
build:
context: ./client-admin
dockerfile: Dockerfile
args:
NODE_ENV: development
environment:
CHOKIDAR_USEPOLLING: "true"
volumes:
- ./client-admin:/app
- /app/node_modules
ports:
- "3000:3000"
networks:
- "polis-net"


Loading