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

Help with how to upgrade JS (Node) dependencies #64

Open
lb- opened this issue Sep 23, 2023 · 16 comments
Open

Help with how to upgrade JS (Node) dependencies #64

lb- opened this issue Sep 23, 2023 · 16 comments
Labels
documentation Improvements or additions to documentation

Comments

@lb-
Copy link
Member

lb- commented Sep 23, 2023

I feel like I should know this but I cannot, despite trying everything I can think of, update the JS dependencies in my existing usage of this Docker environment.

I only have a basic understanding of Docker and I understand that the step Copying node_modules, this may take a few minutes... is copying from somewhere, this copy is obviously cached and contains an outdated version of node_modules.

Please can someone help me know how to update this, last time (earlier this year) I had to completely delete the entire folder and rebuild all Docker content (clone the repo) from scratch.

This time, I have been trying on an off for a week to get this working more simply and I cannot work it out.

How do others update the node_modules that is cached? I am happy to add this to the Readme if someone can point out how they do this.

Things I have tried

  • Outside of the Docker container, update the node_modules and run build etc (this helps for running tests and testing a build but this is not what the Docker actually runs with)
  • ssh-fe and npm install - does not change anything after restarting the Docker containers
  • ssh-fe and npm ci - just crashes/closes the Docker container, never finishes
  • ssh-fe and rm -rf ./node_modules - just crashes/closes the Docker container and it won't start at all again
  • Delete and rebuild the frontend container from scratch, works the first time, after restarting it reverts to some previous cache somehow
@lb- lb- added the documentation Improvements or additions to documentation label Sep 23, 2023
@lb-
Copy link
Member Author

lb- commented Sep 27, 2023

I wonder if #59 is related.

@thibaudcolas
Copy link
Member

thibaudcolas commented Oct 5, 2023

I have a feeling we should drastically simplify this docker-wagtail-develop setup to have much fewer gotchas, and be much lower maintenance. And possibly test simpler "run it all on your computer" options more on Windows to sign-post them more prominently.

As I see it, the Docker setup has two benefits:

  • Isolation / compatibility. Don’t need to trust the system’s Python / Node / scripting.
  • Dev with Postgres. Certainly a plus, but only relevant for a small fraction of contributors.

We’d still get those same benefits with a much simpler "Docker as a VM" setup:

  1. Have a single container for backend and frontend tooling.
  2. Said single container does nothing "on boot". You have to ssh into it to run commands like in any other environment
  3. Go even further and don’t even install anything aside from Python and Node as part of the container’s image build.

That way the "initial install" and "upgrade" steps are the exact same in Docker and out – npm install, etc. There’s more work to do for the initial setup, but it’s the same commands you’d run elsewhere, with less "Docker" magic.

I’d almost suggest we go even further and:

  1. Use SQlite by default. The setup is even simpler with it, and we can document how to run this all with Postgres for those who do need it.
  2. Install pyenv and nvm/fnm rather than Python / Node. That way there‘s not even a need to ever rebuild the container aside from OS upgrades.

The only aspect of this I’m unclear on is how many of the local file system files to have in sync with Docker via volumes. I’d say "all of it" would be the best, but only if Docker performance is good enough for that.

@saevarom
Copy link
Collaborator

saevarom commented Oct 5, 2023

I agree that the node part of this setup is a real pain. I have probably about 50 projects locally on docker and all my problems come from node in one way or another. I kind of hate it 😄

I am not really sold on having such a simplified setup for docker. I don't think it's really a benefit to have such a 'dumb' setup, since you would have to do all the busy work manually and when you for some reason have to delete the container you have to do it all over again.

I have been experimenting with a different base image that is being maintained here: https://hub.docker.com/r/nikolaik/python-nodejs - I could have a go at converting to this base image and see if that works better.

Postgres is of course not essential and not really a part of the problem. If we want to have sqlite as the default, it's quite simple to switch and then have documentation about running this with Postgres.

@thibaudcolas
Copy link
Member

@saevarom perhaps we should start with the "single image" aspect of what I proposed then? That base image is exactly what I had in mind: https://github.com/nikolaik/docker-python-nodejs/blob/main/Dockerfile#L10 as the initial "single container" step

@saevarom
Copy link
Collaborator

saevarom commented Oct 5, 2023

I agree, let's try that out and see where it takes us. I'll set up a branch for this and make a draft PR.

@lb-
Copy link
Member Author

lb- commented Oct 5, 2023

Just a note. We are likely very close to using Node 20 in Wagtail, it will be the LTS in about 20 days from now. It might be worth staring that new exploration with a Node 20 image. https://nodejs.dev/en/about/releases/

Also, any tips for now I can upgrade my Node usage now, with the current setup?

@lb-
Copy link
Member Author

lb- commented Oct 5, 2023

Thank you both for your expertise here.

@saevarom
Copy link
Collaborator

saevarom commented Oct 5, 2023

@lb- You probably have to delete the node_modules volume if you want to totally clear the npm cache.

@saevarom
Copy link
Collaborator

saevarom commented Oct 5, 2023

@lb- Another way would be to go to shell on the web container and try to rm everything inside the node_modules folder, not the folder itself. Then try rebuilding the containers.

@lb-
Copy link
Member Author

lb- commented Oct 7, 2023

Thanks @saevarom I already tried your first suggestion the other week, I am unable to actually just delete the node_modules volume without deleting basically all containers as both the web and frontend rely on them. Here are some screenshots from the Docker UI to show what warnings I get if I try to simply remove that volume.

Screenshot 2023-10-08 at 8 21 08 am Screenshot 2023-10-08 at 8 21 16 am

As noted (see my 'things I have tried'), I did this last week and the right node_modules were there the first time (after building everything again), but then stopping and starting it somehow lead to the wrong node_modules being used again.

Instead, I have now tried your second suggestion and this also did not work for me, maybe I am missing a step, I still somehow have outdated node_modules.

  1. make start (terminal 1).
  2. make ssh-wagtail (terminal 2).
  3. In terminal 2, ran rm -r node_modules/* -> root@35195167c697:/code/wagtail# rm -r node_modules/*.
  4. Stop the docker (terminal 1), exit.
  5. In a third terminal, do a fresh npm ci (completely outside of Docker, not sure if this is necessary but I have no idea what's running npm install or npm ci in any Docker related code).
  6. Now back in terminal 1, run make build, wait a while.
  7. Then run make start.

I am still seeing the old node_modules being used, where the packages case TypeScript warnings.

More details

If I check the version of Stimulus outside of Docker I see the correct version

wagtail on  main [$] via ⬢ v18.12.1 
➜ cat ./node_modules/@hotwired/stimulus/package.json 
{
  "name": "@hotwired/stimulus",
  "version": "3.2.2",
  "license": "MIT",

If I do the same inside the web container I see the old version (this is after a fresh make build using the Wagtail folder as described above.

➜ make ssh-wagtail
docker-compose exec -w /code/wagtail web bash
WARN[0000] The "PYTHONPATH" variable is not set. Defaulting to a blank string. 
root@6aee4486ca3e:/code/wagtail# cat ./node_modules/@hotwired/stimulus/package.json
{
  "name": "@hotwired/stimulus",
  "version": "3.2.1",
  "license": "MIT",

Finally, doing the same in the frontend container, also shows the old version.

➜ make ssh-fe     
docker-compose exec frontend bash
WARN[0000] The "PYTHONPATH" variable is not set. Defaulting to a blank string. 
root@3592731cbfd8:/code/wagtail# cat ./node_modules/@hotwired/stimulus/package.json
{
  "name": "@hotwired/stimulus",
  "version": "3.2.1",
  "license": "MIT",

All of this is after running make build.

@jsma
Copy link
Contributor

jsma commented Nov 11, 2023

Hi all, I've just submitted a related PR in #71 but have not yet touched the frontend container configuration, and then found this thread after the fact ;)

@lb- one of the problems that my PR attempts to solve though is the use of delegated on the volume mounts, which was an incorrect choice and is unnecessary in modern docker compose (it gave the container the authoritative view of the volume, not your host). I discovered this issue when modifying models in the bakerydemo on my host. I could not get the container to detect my changes when running makemigrations over many attempts. Once I removed that option, the host & container are fully synchronized.

Also, the web container does not need to have node_modules, since it does not have node installed in the first place. It's like mounting a populated virtualenv into a node container that doesn't have Python lol ;)

Can you give my PR a try to see if that solves some of the issues you're seeing? It does install a newer version of PostgreSQL so you'll need to repopulate the db. But you'll at least be able to delete the node_modules volume and see host/container sync working.

There is a new-ish docker compose "watch" configuration which may be of use here, but I have not yet wrapped my head around it to get a working proof of concept going:

Here's the example from their documentation:

services:
  web:
    build: .
    command: npm start
    develop:
      watch:
        - action: sync
          path: ./web
          target: /src/web
          ignore:
            - node_modules/
        - action: rebuild
          path: package.json

What this would do is watch for changes in your JS/SCSS source files on the host, then sync those files to the target in the container (where the running npm would gobble them up and spit them out in compiled form), as well as watch package.json and rebuild the container automatically if you change it on your host.

At any rate, I've only reluctantly used node/npm in the past, so I'm unfamiliar with some of the nuances here (especially how Wagtail's assets are organized and built, and how that should be piped into bakerydemo for testing).

As far as the larger conversation, I'd love for this project to be a batteries included project so that any developer working at any level of the stack could spin up a fully functional environment (e.g., elasticsearch...I have a compose configuration that I'll try to port over soon). I would not like to see PostgreSQL removed since that would prevent someone from working on full-text search etc.

@lb-
Copy link
Member Author

lb- commented Nov 11, 2023

Epic. I'll take a look. Just a note - it's @lb- with the hyphen. I'd love the username without, so maybe keep mentioning it and that user will give it over haha.

@jsma
Copy link
Contributor

jsma commented Nov 11, 2023

🤦‍♂️ As someone with a hyphenated first name, you have my sincere apologies! I had tried to insert the at-mention at the beginning of the line and the autocomplete got mangled :(

@lb-
Copy link
Member Author

lb- commented Nov 12, 2023

No problems at all, it's not an issue.

@jsma
Copy link
Contributor

jsma commented Nov 16, 2023

After spending a lot of time on this I think I'm ready to admit defeat when it comes to Node in Docker 🥺

Using node this way (where it isn't the entire app, it's just a tool generating files for another app) doesn't appear to have any good solutions.

@lb- after rereading your comments above, I think one part of the issue may have been due to an overlap with a node_modules that was built on your host and then the container also modified it, which will lead to problems. In my own project, I had to delete node_modules on my host and learn to do any npm commands within the container, so the container is the source of truth of what goes in package.json and package-lock.json (and ultimately, the container's node_modules).

One workaround I've seen is to install node_modules in a parent directory e.g. /code/node_modules so that it is independent of the host and won't be masked by a volume mount targeting /code/wagtail.

Unfortunately, that does not work in this case because Wagtail has hard-coded relative imports from node_modules, which means that node_modules can only exist at exactly one location: /code/wagtail/node_modules. For just one example:

@import '../../../node_modules/tippy.js/dist/tippy';

Something basic like this will of course also not work:

WORKDIR /code/wagtail
# Either bind mount package.json/package-lock.json to WORKDIR or COPY wagtail /code/wagtail, same result
RUN npm install

The compose volume mounts will eventually mask /code/wagtail resulting in an empty/non-existent node_modules (if there is a host-generated node_modules, this now appears in the container but will likely generate errors due to cross platform compiling issues):

volumes:
- ./wagtail:/code/wagtail:delegated,rw

The docker compose watch feature will not work either since webpack needs to watch the wagtail folder, recompile stuff and write that out to the volume that is then shared with the container running the demo. watch only works for syncing files into a container where stuff gets built under the assumption that the container is the main app, there aren't any volume mounts involved where this gets synced back out to other containers.

The "all in one" Dockerfile mentioned above, also does not solve the problem. We still need some way of getting the local files (and any real-time changes) into that container, which requires mounting a volume which again will mask anything at that target path in the container. It's very chicken vs egg where we can't get the project into the container and then get node_modules into the project.

I can see why the current setup gave up and resorted to rsync-ing every time :(

I'm unfamiliar with the dark arts of node, perhaps there is a way to change the imports in Wagtail to be less dependent on node_modules being in a very specific directory vs in any parent directory, but barring that we're at least stuck with rsync for the time being. I'll try to wrap up my work and implement the lesser of evils option (rsync every docker compose up?) to try to finalize #71 unless someone else has some ideas.

@lb-
Copy link
Member Author

lb- commented Nov 21, 2023

Thanks @jsma - I really appreciate your help here. If I knew more about this I would give it a go, maybe I should try to learn a bit more.

I also tried your PR #71 but could not get that working locally sorry, but maybe I have missed a step.

For now I think I will just keep using a fork of #66 from @saevarom - it's noisy but works a bit better than the current main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants