feat: recreate node_modules if not existing on host machine

[soey]

When the containers are built npm install is run in the container. This installs node_modules in the container itself .. so far so good.

BUT when the containers are started we mount the hosts current directory (app) into the containers app directory (see docker_compose.override.yml) to have all our code executed in the container.

That makes the node_modules installed in the container itself invisible for the container. The container is now relying on a proper installation of node_modules in the hosts app directory. That is unwanted as the container itself relies on installations on the host system.

Without this we have to either run npm install locally or from the container itself to fix it. With this the assets container installs node in the hosts app directory if it doesn't exist.

To be discussed:

• Maybe we should inform the user if they are possibly using an already existing (maybe outdated) node_modules
• We could be even more aggressive and delete the existing node_modules

[mattwr18]

@soey Sorry it took so long.

I am just going to comment here because I would like to continue the conversation and see what you think about my solution vs requesting you change your code or approve it.

[mattwr18]

From my research, I think we came to more or less the same conclusion. What I understand is that during the build, we copy the package*.json and then run npm install to install dependencies, then we copy everything to the image. However, in our docker-compose.override.yml, which is used in development, we mount the volumes like so:

.:/app

From my understanding, volumes, are bi-directional, meaning that changes to the local files will be reflected in the container where the volumes are mounted. If one already has the node_modules locally, they will be mounted in the container and can be outdated as the build image node_modules will be overridden.

The issue I have with this small change, as I understand it, is that it will check for the presence of the node_modules directory locally and if not present run npm install inside the assets directory, it does not reflect any changes to packages updated and rebuilt, but not explicitly installed either locally or inside the container after it's started.

If you update a package on this branch, see the results of:

docker-compose down
docker-compose up --build
docker compose exec -it assets bash
npm ls

I remember we talked about this in our last standup and I was confused why this has become an issue only recently and why it only affects the assets service. I looked through our Slack history and several of us have been complaining about this issue for years. I ran into this when I onboarded back to the project two years ago, I had just forgotten about it.

I spent quite a bit of time going through stack overflow issues trying out different approaches. Some of the things I tried were:

• Mounting an anonymous volume for node_modules and then when a change occurs running docker-compose up --build --force-recreate --renew-anon-volumes ... Does not crash either, the node_modules seem to be copied and not overwritten correctly from the build step, but the package-lock.json is not updated and running npm list <package> returns (empty) so it's not clear to me what happens in this case
• Name volumes, honestly can't remember
• docker-compose watch, which allows to ignore any paths, so we can ignore node_modules. Also allows to rebuild when package.json changes so no need to restart. Really cool, unfortunately CSS changes are picked up, but they are not reflected in the browser.

What finally worked for me and I would suggest is that we do not create a mounted volume for everything
We want to watch for any changes to our JS and CSS files and build when this happens. So we can simply do this:

assets:
  <<: *x-dev-defaults
  volumes:
      - ./app:/app/app			

[soey]

Yes, you are absolutely right. That proposal of mine will still have the issue that the folders will get out of sync as you described when you miss to update them on rebuild etc. What you propose is way better if it works. I also had this idea at first - maybe I missed something as being not able to make it work. I will just try to change it the way you propose and adapt this if it's solving all my issues locally.

[soey]

Ok - I tried with your approach but it seems that the esbuild is not properly watching the changes now. Please test the following:

1. Spin up the containers
2. Delete the generated css and or js in app/assets/builds/
3. Change some source css or js file in the app/assets

With the current solution: esbuild is watching the changes and regenerates the css/js builds
With your proposal: I don't see any regeneration

Still I would be happy if your solution would work. Currently I don't understand why not..

So another solution would be that we always override the delete & rebuild the linked node_modules whenever we spin up the assets container

[mattwr18]

hey @soey I tested your workflow with my proposed change. I deleted the app/assets/builds/applications.css, this would not work to trigger the npm run build:css as this is the output file for the build. If you modify the app/assets/stylesheets/application.css, it does trigger a build, for example I updated the import to only import one components css files and refreshed the page to verify all styles were broken.

If you modify any .css files in the components directory, you will also be able to see the changes with a refresh without the need to run npm run build:css or stopping and restarting the docker containers. This is also true if you modify any .js files in the app directory. There are some .js files in the root directory, but they are config files so I think you would need to restart the server for those changes to take effect. Maybe, with ctx.rebuild() in the esbuild config instead of ctx.watch(), it would work, but I don't think it's worth the change.

[soey]

Sounds good. Feel free to commit that changes - I will do some tests then on my side to verify...

[mattwr18]

Ok, I pushed up the change, but I'm not 100% happy with it. I did some more testing and will share my findings here.

So, updating a package and rebuilding with docker-compose up --build and then bashing into the assets container and running npm ls, I can see there is no warning for invalid package with a mismatched version. npm list <package> returns:

/app
└── (empty)

if I run cat package-lock.json | grep <package>, I can see it resolves to the lower version.
If I cat node_modules/<package>/package.json I can see that it is the higher version.

If I run the same commands in one of the services that mounts the entire projects' path to the app directory, app service, for example, I can see that I do have an error when running npm ls:

npm ERR! invalid: [email protected] /app/node_modules/esbuild

npm list esbuild

/app
└── [email protected]  invalid

npm ERR! invalid: [email protected] /app/node_modules/esbuild

the package.json is the same
the node_modules have the lower version.

The package-lock.json make sense since it copies it from our local machine to the docker image and since I have not run npm i locally, it cannot be updated. I think that we want to commit changes to the package-lock.json, so I think we need to run npm i locally when we update any dev dependencies.

Gotta run now, but I still have other open questions like why the issue was only observed in the assets service?

[soey]

If node_modules is being built from within a container into the local directory (after linked) how can there be the mismatch?

After playing around for myself I would really go with a solution where everything is handled from within the app container to keep consistency.

[mattwr18]

If node_modules is being built from within a container into the local directory (after linked) how can there be the mismatch?

My understanding is that the node_modules are copied into your local file system the first time the containers that contain the volumes are spun up. The mismatch occurs because we don't run npm install inside any containers once the update to a package in the package.json occurs.

After playing around for myself I would really go with a solution where everything is handled from within the app container to keep consistency.

I appreciate your concern with my proposed solution. I think there are benefits and potential downsides to both approaches. I think we can find a third solution that addresses both of our concerns.

Your concern, as I understand it, is with setting up a volume for the assets service, which does not watch any changes outside the app directory. It's my understanding that the assets service does not need to watch changes, or have access to them, outside the app directory, but I think it's still a valid concern.

My concern is that the solution you proposed does not keep the packages up to date so, if you update them, you need to remember to manually run npm install from inside a container, or locally, if possible.

You suggested running npm install every time we fire up the assets container in Slack. I think this would be a valid solution, but I don't think it's great dev experience. It takes time to run npm install every time we spin up our containers and we don't upgrade that often.

I was thinking of keeping your changes as they are so that if there is no node_modules directory locally, we run npm install. Then I propose updating the dev script so that if you do pull in changes to a dependency and forget to run npm install, or don't notice that you should, starting up the assets container would result in an error.

the script would now look like:

"dev": "npm ls && run-p -l 'build:** -- --watch'",

This also has the benefit of fixing some issues that we have currently that are unresolved. For example, we have an unmet peer dependency:

UNMET PEER DEPENDENCY prop-types@^15.7.2

npm ERR! peer dep missing: prop-types@^15.7.2, required by @yaireo/[email protected]

It is also much quicker to run than npm install.