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

Upgrading npm dependencies (just upgrade_node_requirements) makes node container stop working #289

Closed
codyfletcher opened this issue Jun 1, 2023 · 3 comments

Comments

@codyfletcher
Copy link
Contributor

codyfletcher commented Jun 1, 2023

How to reproduce the issue.

  1. Create a new project (following the README.md)
just start
just upgrade_node_requirements
  1. Stop containers (ctrl-c)
  2. Attempt to bring containers up: just start

The above command fails with:

node    |
node    | > dev
node    | > vite --config src/config/vite.config.js
node    |
node    | sh: 1: vite: not found
node exited with code 127

One potential solution

All I could find to resolve this error was to prefix the vite command in package.json with npx:

    "dev": "npx vite --config src/config/vite.config.js",

I'd submit a PR, but wasn't sure it was inline with how you would solve it. Very interested in your thoughts! This is an AMAZING project, by the way. 👍👍👍

A non-code solution

This might be akin to saying "don't end up here", but...

  1. Stop containers, if running (ctrl-c)
  2. Delete containers: docker ps -a | grep "django-base-site" | awk '{print $1}' | xargs docker rm
  3. Delete volume: docker volume ls | grep "django-base-site" | grep "node_data" | awk '{print $2}' | xargs docker volume rm
  4. Bring containers back up: just start
@codyfletcher codyfletcher changed the title Upgrading npm dependencies (just upgrade_node_requirements) make node container stop working Upgrading npm dependencies (just upgrade_node_requirements) makes node container stop working Jun 1, 2023
@epicserve
Copy link
Owner

@codyfletcher,

Thank you for reporting this bug. I've pushed up a fix that removes Vite, ESLint, and Stylelint from package.json and instead installs them globally as defined in the Dockerfile for the node service.

I want to note that after you run just upgrade_node_requirements, you need to run docker compose build, and also, for some strange reason, you need to run docker compose run node npm i. If you have any ideas on removing the docker compose run node npm i step, I'm all ears. One thought is to remove the node_data volume defined here. I think I originally picked up that configuration from a blog post on how to make the node_modules directory available locally and in docker. The downside is that directory would no longer exist unless you installed the node dependencies locally. I'm guessing that could be part of the issue.

@epicserve
Copy link
Owner

@codyfletcher,

I've pushed up two more commits that should fix all the weirdness with having to run docker compose run node npm i after a build because it removes the local docker volume. I've also switched back to using npx, which should work again because the node packages should always reside in the docker image.

@codyfletcher
Copy link
Contributor Author

@epicserve – Great job and great solution(s)!

I also like the iterative approach you took to finding the current solution. It feels great to have the npm dependencies outside of the Dockerfile, but you gotta earn it sometimes!

I've got a bunch of thoughts I want to run by you and find a way to possibly pool our resources! I've been using Docker forever, but this is the first time I've found pytest to actually be performant during development (via docker run --rm in PyCharm) in a non-trivial project. Great work!

I've got a couple of small PRs to send over too!

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

No branches or pull requests

2 participants