Fix dependencies vs devDependencies

[zackkrida]

Fixes

Fixes #400 by @zackkrida

Description

Moves all dependencies required to run npm run build into the dependencies list, so we can use npm ci --only=prod to install dependencies in our Dockerfile (see #388).

Testing Instructions

1. npm ci --only=prod && npm run build
2. Verify that the app built successfully
3. (optional) Click around the application after npm run start
4. (optional) npm run test to verify tests pass

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[krysal]

Removing the node_modules folder and running npm ci --only=prod gave me an error:
sh: husky: command not found. Easy to solve doing first npm install --only=prod (I also ran npm audit fix to solve some moderate vulnerabilities but... not much difference really 🤷🏻‍♀️)

Edit: actually husky is mostly for development, do we need it in production for something?

[zackkrida]

@krysal

running npm ci --only=prod gave me an error: sh: husky: command not found

hm, that's surprising to me because I moved husky to dependencies! It does need to be in prod because it's used to do a postinstall script that patches a package used in production.

[rbadillap]

You might not need husky in dependencies.
Based on the feedback received in #388 we can run npm ci --only=production --ignore-scripts so we avoid prepare and postscript executions.

Now the question: is it ok to ignore them? I believe I'm ok with prepare but postscript seems to be still needed in production.

So I have two workarounds:

1. Change the Dockerfile in Improve Dockerfile and enable pipeline to automate docker image generation #388 and set a blank prepare script manually by running

# remove prepare script
RUN npm set-script prepare ""

# install without ignoring scripts
RUN npm ci --only=production

2. Use a .js file to have more control of what we want to be executed.
   Taking this example we can do something like this:

{
  "scripts" : {
    "install" : "scripts/install.js",
    "postinstall" : "scripts/install.js"
  }
}

and scripts/postinstall.js

if (process.env.NODE_ENV !== 'production') {
  // .. husky can be called here
}

What are your thoughts about this?

[dhruvkb]

Tailwind also should be a dev-dependency, the build step will generate the actual CSS used in prod.

[dhruvkb]

Same for PostCSS and Sass.

[zackkrida]

There's an unclear distinction with 'devDependencies' on whether they should be 'deps not included in the production bundle' or 'deps needed for local development'. to put the question differently:

If we run npm install --production && npm run build, should the build work?

cc: @WordPress/openverse-frontend

[sarayourfriend]

Well, when you add in an SSR server that needs to be able to build things inside the container, the distinction gets muddied. Production dependencies should include everything required to "run the app." If we built a standalone package with styles that docker was able to consume then yeah, tailwind would be a dev dependency because the only production dependency would be its output. But because our "production" release includes building the app from scratch inside the container then we need to include some of these dependencies that seem to cross the boundaries.

If we run npm install --production && npm run build, should the build work?

I think yes.

[zackkrida]

I agree 👍

[rbadillap]

If we run npm install --production && npm run build, should the build work?

From a DevOps perspective, it shouldn't.
Build your application is one step before getting it up and running. Being very simplistic, production doesn't care how you came to create a distribution package.
They are only interested in having the necessary files and commands to be able to run freely and independently.

So the question we should make is: what packages should be placed in dependencies to make sure to run the app as soon as I have the dist folder ready, that's how I consider nuxt should be one of the dependencies (because we need to run nuxt start) but husky shouldn't.

In addition, that's why I'm using two stages in Dockerfile. The first stage requires devDependencies to build the application, but, the Production stage doesn't need them, that's why I'm transferring the dist folder from one stage to the other, because that's the only assets I'm care of.

More info: https://www.docker.com/blog/speed-up-your-development-flow-with-these-dockerfile-best-practices/

[sarayourfriend]

Definitely agree that husky shouldn't be a regular dependency. Nothing it does is necessary for building the app, it's just meant to run developer level verification tools.

If you're doing a two-stage docker build like that then yeah, you can and should move anything that isn't necessary for after nuxt build is run I guess.

[krysal]

I like how the approach of @rbadillap sounds with two stages build, keeping the running production environment lean. Moving dependencies around I got this warning message in one opportunity:

WARN Module @nuxtjs/tailwindcss not found. Please ensure @nuxtjs/tailwindcss is in devDependencies and installed. HINT: During build step, for npm/yarn, NODE_ENV=production or --production should NOT be used.

[zackkrida]

I'm going to close this, since we will be following a multi-stage build approach I think it will be simpler to reduce size there rather than fuss too much over devDependencies vs. dependencies.