-
Notifications
You must be signed in to change notification settings - Fork 22
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
Reworked Docker configuration and minor fixes #12
Conversation
Removed the named volume due to a issue with updating node_modules. Instead of working around node_modules this now imports all needed directories and files individually and doesn't touch the rest. |
Dockerfile
Outdated
WORKDIR /app | ||
|
||
RUN apk --no-cache add git | ||
RUN npm install --global pnpm | ||
RUN npm install -g npm@latest |
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.
Installing the latest version of npm
may cause containers to unexpectedly break:
> [workspace base 4/5] RUN npm install -g npm@latest:
0.923 npm ERR! code EBADENGINE
0.924 npm ERR! engine Unsupported engine
0.924 npm ERR! engine Not compatible with your version of node/npm: npm@10.2.1
0.924 npm ERR! notsup Not compatible with your version of node/npm: npm@10.2.1
0.925 npm ERR! notsup Required: {"node":"^18.17.0 || >=20.5.0"}
0.925 npm ERR! notsup Actual: {"npm":"9.5.1","node":"v18.16.1"}
0.926
0.927 npm ERR! A complete log of this run can be found in:
0.927 npm ERR! /root/.npm/_logs/2023-10-24T06_51_39_300Z-debug-0.log
Admittedly, the above problem is being caused by my local node:lts-alpine
container being out of date, but is there a compelling reason to install the latest version of NPM rather than riding the image-provided version?
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.
No, just preference as it was used to install pnpm.
Which is using the official script now, not npm. Removed updating npm.
On that note:
I pinned the version of pnpm as well to avoid future potential breaks as with npm 9 -> 10.
For now to the version specified in package.json
.
This is not a requirement. It will install the version from package.json
which is then accessible in /app
anyway.
For that I moved the install of pnpm into the dependency stage of Dockerfile
.
Inside of /app
pnpm --version
now returns the version specified under package.json
.
docker-compose.yml
Outdated
- ./docs:/app/docs | ||
- ./news:/app/news | ||
- ./api_versioned_docs:/app/api_versioned_docs | ||
- ./api_versioned_sidebars:/app/api_versioned_sidebars | ||
- ./src:/app/src | ||
- ./static:/app/static | ||
- ./custom.css:/app/custom.css | ||
- ./package.json:/app/package.json | ||
- ./api_versions.json:/app/api_versions.json | ||
- ./default-sidebars.js:/app/default-sidebars.js | ||
- ./docusaurus.config.js:/app/docusaurus.config.js |
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.
This seems fine, but I worry a bit that we may forget to add new important files to this. Not a blocking review comment but something I want to bring up in general.
I'm not a JS dev so forgive the possibly obtuse question: is there a way to change node_modules
to live in another directory entirely (say /opt/node_modules
) so that we can just sync in all of app
and just not care? Or, in other words I suppose, can we just tell pnpm to install deps globally somehow?
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.
I'm not a JS dev either, but maybe someone can chime in / correct later. From previous research most ideas are to block node_module
from importing by making it a separate volume.
- named volume:
- prevents node_modules from being shared between host/container
- but to update dependencies you need to
docker volume rm <dalamud-docs-...>
before rebuilding the image
- anon volume:
- prevents node_modules from being shared between host/container
- needs a
docker compose down
(to remove the association between container and volume) ordocker compose down -v
(docs) to skip below - since the volume still exists needs
docker volume prune
ordocker volume rm <anon-volume-id>
- Note: Like named volume but more things to keep track of
- node_modules in parent directory with empty named volume for
node_modules
- prevents node_modules from being shared between host/container
- doesn't interfere with builds
- didn't run with
pnpm
as it couldn't find node_modules; did work if starting withnpm
- no additional steps to update dependencies
I'm not sure what the consequences between starting with npm start
or pnpm start
are. I would assume that there's not much of a difference, (looks like it just calls docusaurus <command>
(as defined in package.json
) but am missing experience there.
Since at the momentI can't build on windows (#11), I wanted to use Docker.
As mentioned this ran slow on my system. I changed the Docker setup and saw major speed increases.
This PR seeks to implement them in the main repo if you are interested.
I can't pinpoint what made it run slow before.
My best guess would be Docker Desktop for Windows and file IO between Windows and Docker on WSL2.
Comparison with my setup (Win + Docker WSL2):
There are differences in operation compared to before as well.
Build dependencies are baked into the image, thus the Image size change.
This enables use of pnpm cache for rebuilds and (my guess) contributed to the faster execution times.
To update dependencies just
docker compose build
with a newpnpm-lock.yaml
.Workaround for
node_modules
If we create a volume of the whole dalamud-docs directory
node_modules
will be deleted in the container as it does not exist on the host. To avoid this/app/node_modules
is bound to named volume separate of the host filesystem.Bonus: You can have a node_modules directory on host without it spilling to the docker container and causing this:
Despite some efforts to let docusaurus listen to FS events of the host, this still didn't work with the Windows/Docker setup.
The default behavior now includes
--poll 1000
.Note: Some IDE, e.g. WebStorm, still don't trigger the FS events by default. For me changing this setting didn't hot reload either, that's why I'm defaulting to poll.
This also includes 2 minor fixes that were mentioned in #11 as well.
Constructor
fails (apparently some OS are case sensitive and other aren't)<br>
brackets that start complaining in the docusaurus v3-betaFurthermore for if anyone who runs into this error again.
At some point randomly, no changes to the dockerfiles, this error happend on startup:
Restarting the OS (in my case just WSL) did solve the issue again.