craft icon indicating copy to clipboard operation
craft copied to clipboard

make dev doesn't run npm install on first run

Open dlindberg opened this issue 2 years ago • 6 comments

Describe the bug

When working with a project that uses a version controlled package-lock.json file make dev will not cause npm_install.sh to run npm install because the directory node_modlues always exists because it is a volume delegate in docker-compose.yml.

To reproduce

Steps to reproduce the behaviour:

  1. Clean pull of a project with a version controlled package-lock.json
  2. make dev
  3. Vite container crashes

Expected behaviour

Vite container runs npm install and starts

dlindberg avatar Jun 03 '22 14:06 dlindberg

To solve this for Composer's vendor/ dir, I look for the autoload.php which isn't generated until after the composer install is done. I wonder if there is an equivalent for npm?

khalwat avatar Jun 03 '22 14:06 khalwat

Yeah, that's what it looks like, and managed to respond faster then me writing the pull request...

dlindberg avatar Jun 03 '22 14:06 dlindberg

Do you think checking for an empty directory is the best solution for NPM, or is there a file of some kind that gets written like autoload.php for Composer for NPM?

I don't know the answer...

khalwat avatar Jun 03 '22 19:06 khalwat

I’m not aware of analogue to autoload.php though I spend more time with PHP then JavaScript so I could be missing a better event to look for that I am also not aware of. My thought was that because we aren’t in a loop waiting for install to complete like the queue container just checking for empty is probably adequate. And simple solutions are often good solutions.

The down side is you could have an old node_modlues folder that doesn’t match up with the current version of package-lock.json and the container would have no idea. Which is also true for the current composer.json handling. I like the move away from just always running install because it is a lot faster.

The other option I considered was using a environment variable flag with docker-compose up. I’m currently using that strategy to trigger Servd’s database pull command which won’t work until after Craft has successfully installed with the plugin and I want to avoid unnecessarily downloading the database. Using the command INIT=1 docker-compose up lets me pass the INIT variable into the docker-compose.yml file:

services:
# •••
    php:
# •••
        environment:
            INIT: ${INIT:-0}
# •••

Which lets you access it from the container by checking if INIT is something other than 0 in the shell scripts using if [ 0 != "${INIT:-0}" ]. I have make do that when it has to copy the example.env file into .env (it also prompts for some project secrets).

That approach is quite a bit more complicated but would let you explicitly say when you want to run the installer and when you don’t from the make file. So you could add some more commands to the make file to handle git checkout and git pull and so forth; but they wouldn’t work with desktop clients. Which brought me back to, checking empty is probably adequate if imperfect. I did also add a make command that I’m trying out that dumps the vendor and node_modules directories before starting (but not the lock files) so it picks up the versions in the lock files from whatever commit you just pulled in. (I do not have a good name for this but am calling it check.)

dlindberg avatar Jun 04 '22 02:06 dlindberg

To solve this for Composer's vendor/ dir, I look for the autoload.php which isn't generated until after the composer install is done. I wonder if there is an equivalent for npm?

Hi! I'm getting the same issue. Vite won't run due to: /bin/sh: /var/www/project/npm_install.sh: not found. Can you elaborate what needs to be done to get around the issue? Thanks!

jayhlee avatar Aug 02 '23 00:08 jayhlee

@khalwat I believe the problem here is that npm_install.sh is a bash script, but we don't have bash in this context. If I updated the hashbang, it seems to work: https://github.com/nystudio107/craft/pull/94

timkelty avatar Oct 26 '23 10:10 timkelty