Skip to content

#36 Add local docker setup with hot-reload #51

Open
LeoQiu981112 wants to merge 9 commits intomainfrom
features/addDockerFile
Open

#36 Add local docker setup with hot-reload #51
LeoQiu981112 wants to merge 9 commits intomainfrom
features/addDockerFile

Conversation

@LeoQiu981112
Copy link
Collaborator

Setup OpenBC to run in docker container #36

Added corresponding docker files for project, with hot-reload and pre-render
updated wiki manually

@umsu2
Copy link
Member

umsu2 commented Dec 26, 2023

there is many issues with this approach.
it is not a good production build because you shouldnt run npm install in prod build. better approach would be to build in pipeline or multistep build then copy the final files into the prod build. also the file mounting is a little weird not sure what is happening there.

@LeoQiu981112
Copy link
Collaborator Author

there is many issues with this approach. it is not a good production build because you shouldnt run npm install in prod build. better approach would be to build in pipeline or multistep build then copy the final files into the prod build. also the file mounting is a little weird not sure what is happening there.

This ticket is meant for dev/local environment only, and the mounting is used for enabling next.js hot reload as per the ticket requirement. The ask/requirement is as follows.

Task
Setup OpenBC to run in Docker Container
Update 'npm start' command to run docker instance
Ensure that changes to files are deployed into the container (hot reload)
Create Wiki on how to install Docker locally
Deployment into container should be based on pre-rendered pages
Note: This change is only to local development. Wiki pages cannot support docker.

Co-authored-by: Alja Čekada <112892699+alchuu00@users.noreply.github.com>
next.config.js Outdated
basePath: process.env.NODE_ENV === 'production' ? '/openbc-web' : '',
output: 'export',
distDir: 'dist',
basePath: process.env.NODE_ENV === 'production' ? '' : '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this to be empty string only. Remove the ternary.

import React from 'react';
import { Typography } from '@mui/material';
import Container from '@mui/material/Container';
import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add this?

Dockerfile Outdated
COPY --from=builder /app/node_modules ./node_modules
COPY --from=builder /app/package.json ./package.json
CMD ["node", ".next/standalone/server.js"]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure there is no extra line at end of file.

Dockerfile Outdated
@@ -0,0 +1,24 @@
FROM node:18-alpine as base
RUN apk add --no-cache g++ make py3-pip libc6-compat
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need all these dependency?

next.config.js Outdated
typescript: {
tsconfigPath: './tsconfig.json',
},
webpackDevMiddleware: (config) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we need this?

I get a warning that this is not recognized.

@LeoQiu981112 , @alchuu00 , @umsu2

Copy link
Member

Choose a reason for hiding this comment

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

this seems like it's used for dev watching purposes. not sure how this will work in production.
working with docker + watching the files is not trivial. what happens with file mounting is that the the node runtime in your local system is doing the transpiling, then gets mounted over to the docker node runtime which runs the code.
what we probably want is to have the build/run command run from the docker container instead of your own system. the best litmus test to have it setup properly is to actually not have node.js installed on your machine. can you use docker, docker compose to run the app in container. that's the only way to garantee consistency in terms of build across dev machines/production

Copy link
Collaborator

Choose a reason for hiding this comment

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

Trying to hot reload and having it mimic product might be to much work right now.

Perhaps we could forgo hot reload for now and just focus on building it like production.

Create a ticket to look into hot reload.

@SamHuo213
Copy link
Collaborator

@LeoQiu981112 , still looking at this.

Hold off on changes.

Dockerfile Outdated
Comment on lines 3 to 11
WORKDIR /app
COPY package*.json ./
EXPOSE 3000

FROM base as builder
WORKDIR /app
RUN npm install
COPY . .
RUN npm run build
Copy link
Member

Choose a reason for hiding this comment

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

not sure why the base exists without having even the src file in there
i'd recommend taking a look at this https://medium.com/@maheshwar.ramkrushna/optimized-docker-multistage-build-for-node-js-application-deployment-with-nginx-17655b9e62c1
the idea is pretty similar
your base has the source code copied over with the build command. which builds the artifact
the second stage is just copyinig over the build artifact over to the production image which should be as minimalistic as possible with the least amount of permissions (similar to what you have below)

@SamHuo213
Copy link
Collaborator

SamHuo213 commented Jan 9, 2024

@LeoQiu981112, @umsu2 ,

The current configuration is confusing. Could we simplify it down?

docker-compose.yml

image

next.config.js

image

Dockerfile

image

@LeoQiu981112
Copy link
Collaborator Author

@LeoQiu981112, @umsu2 ,

The current configuration is confusing. Could we simplify it down?

docker-compose.yml

image

next.config.js

image

Dockerfile

image

simplified the configs as requested. I think for windows systems might need to run npm run lint first before building

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants