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.
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.
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.
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.
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 pruneordocker 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
pnpmas 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 buildwith a newpnpm-lock.yaml.Workaround for
node_modulesIf we create a volume of the whole dalamud-docs directory
node_moduleswill be deleted in the container as it does not exist on the host. To avoid this/app/node_modulesis 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.
Constructorfails (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.