Ensure that npm is not run in production mode

[nullobject]

This PR fixes a mistake I made when previously trying to fix up our npm install process.

When we install packages we want to force npm to not run in production mode. This is because we also require development dependencies to be installed on our production servers.

[yob]

So this should install packages from both dependencies and devDependencies?

[nullobject]

So this should install packages from both dependencies and devDependencies?

Correct. Previously we were getting around this by chucking all our dependencies into devDependencies. But I don't think that is the best solution.

[nullobject]

--production=false forces npm to install dev dependencies, even if NODE_ENV=production.

See this thread for more info.

[yob]

Cool. I might apply this change to staging and run a deploy to confirm it works as we expect

[yob]

Unfortunately the deploy to staging failed.

        remote:   common:on deploy {
          remote:     on deploy {
          remote:       conversation:set up newrelic configuration {
          remote:         Writing newrelic config
          remote:       } ✓ conversation:set up newrelic configuration
          remote:       common:when path changed {
          remote:         5 changes within #<Babushka::Parameter:47125096280300 path: /(app|lib|vendor)\/assets\/|config\/locales\/tc_js...>:
          remote:         1     2       app/assets/javascripts/views/draft_link_tooltip_view.coffee
          remote:         4     0       config/locales/tc_js/es.yml
          remote:         1     0       config/locales/tc_js/fr.yml
          remote:         1     0       package.json
          remote:         0     778     vendor/assets/javascripts/clipboard.js
          remote:         conversation:webpack compile during deploy {
          remote:           conversation:npm packages installed {
          remote:             meet {
          remote:             }
          remote:           } ✗ conversation:npm packages installed
          remote:         } ✗ conversation:webpack compile during deploy
          remote:       } ✗ common:when path changed
          remote:     } ✗ on deploy
          remote:   } ✗ common:on deploy
          remote: } ✗ common:post-receive.git_hook
          remote: You can view a more detailed log at '/srv/http/theconversation.com/.babushka/logs/common:post-receive.git_hook'.
          To 206.51.240.238:~/current
             0c4b2a8f92..4431755ada  HEAD -> master

When I look at the logs, it seems the clipboard package was installed, but npm ls --production=false isn't returning what we expect:

> npm ls --production=false
...
npm ERR! extraneous: echo-js@1.7.3 /srv/http/theconversation.com/current/node_modules/echo-js
npm ERR! extraneous: i18next-client@1.11.4 /srv/http/theconversation.com/current/node_modules/i18next-client
npm ERR! extraneous: resolve-url-loader@2.1.0 /srv/http/theconversation.com/current/node_modules/resolve-url-loader
npm ERR! extraneous: sleep@5.1.1 /srv/http/theconversation.com/current/node_modules/sleep
theconversation.com@staging STAGING
> echo $?
1

[nullobject]

I checked, it failed because npm ls found extraneous packages. 🤔

[nickbrowne]

Correct. Previously we were getting around this by chucking all our dependencies into devDependencies. But I don't think that is the best solution.

We've just been following the suggestions in webpack/webpack#520, where "runtime" dependencies go in dependencies, and non-runtime dependencies go in devDependencies. The naming of the two dependency groups is unfortunate, but to me it makes sense to keep non-runtime dependencies separate.

[yob]

I checked, it failed because npm ls found extraneous packages.

So it would've failed under the old dep as well?

[nullobject]

So it would've failed under the old dep as well?

No. We were forcing --dev before which doesn't check for extraneous packages.

We could do a npm prune before we run the npm ls --production=false?

That way any old packages would be removed before we check and install new ones.

[nickbrowne]

I'm not sure, but here's what the old code was doing

dep 'npm development packages installed', :path do
  met? {
    output = raw_shell('npm ls --dev', :cd => path)
    output.ok?
  }
  meet {
    shell('npm install --only=dev', :cd => path)
  }
end

[nullobject]

@nickbrowne That code would never install anything in dependencies. Only devDependencies were being installed. That's why I changed it.

[nickbrowne]

Yup, but it was intentionally done that way

[yob]

It seems like we might need to have a philosophical discussion about how we want to treat dependencies and devDependencies.

Given tc is currently unshippable, I vote for reverting back to the old style (npm install --only=dev) while we have that discussion.

[nullobject]

@yob Can we try it one more time with my tweak first?

[yob]

I think I'd rather just revert as a short term solution. I'm not across the dependencies/devDependencies issues and it seems like both approaches can work, but we should agree on a consistent approach before continuing down this path.

[nullobject]

Ok. TC already has lazysizes in dependencies, so we will probably need to move that into devDependencies to keep it working.

[yob]

Ok. I'll open a PR on TC hat does that - can you prepare the revert to babushka?

[nullobject]

Sure, reverting...

[nullobject]

I'm not going to change anything else with this, now that we're going to dockerise our apps.