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

Added Docker environment setup #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nklyshko
Copy link

Replace for #123

Comment on lines +4 to +5
ADD . .
RUN npm install
Copy link
Member

Choose a reason for hiding this comment

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

To use layer caching more efficiently, the best practice is to first COPY "package.json" and "package-lock.json", then RUN npm install, and only then copy the rest of the files with COPY . .. See https://nodejs.org/en/docs/guides/nodejs-docker-webapp/ to see the pattern (don't forget to add .dockerignore) and https://docs.docker.com/get-started/09_image_best/#layer-caching for an explanation why this approach is better.

Also please use COPY instead of ADD, the reason is here: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy

@@ -0,0 +1,7 @@
FROM node:17
Copy link
Member

Choose a reason for hiding this comment

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

Please use

Suggested change
FROM node:17
FROM node:16

instead. See https://nodejs.org/en/about/releases/ - odd-numbered (9, 11, etc.) releases become unsupported after 6 months and their use in production is discouraged.

- Install docker and docker-compose
- `git clone --recursive https://github.com/kaitai-io/kaitai_struct_webide`
- `docker-compose build && docker-compose up -d`
- Go to [http://localhost:8000/](http://localhost:8000/)
Copy link
Member

Choose a reason for hiding this comment

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

Please use 127.0.0.1:8000 instead of localhost:8000.

The "automatic reload when a local change is detected" feature works only for the 127.0.0.1 hostname:

if(location.hostname === "127.0.0.1")
checkModifications();

Also using localhost instead of 127.0.0.1 caused some 1 sec delay (but I have no idea what the cause was, and whether it still persists), see

print "please use 127.0.0.1:%d on Windows (using localhost makes 1sec delay)" % PORT

(this is a different file for different purpose, unused by the current Web IDE version, but whatever)


Actually for the "automatic reload" to work and also for easier development, it would be nice if the Docker image didn't have to be manually rebuilt every time a change is made to the source files. This is certainly achievable with Docker using bind mounts, see https://docs.docker.com/get-started/06_bind_mounts/.

node serve.js --compile is already capable of watching file modifications and recompiling the app, so just creating the bind mount should be enough I guess.

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.

2 participants