Install devDependencies by default #519

jmorrell · 2018-02-16T01:54:21Z

Will fix #431 by making the default behavior to:

install dependencies + devDependencies
run the build scripts
cache node_modules for the next build
prune devDependencies from node_modules

If a user needs to customize this behavior, they should be able to:

Only install `dependencies`, the current default behavior

You can get this behavior by setting the following env vars:
heroku config:set NPM_CONFIG_PRODUCTION=true YARN_PRODUCTION=true

Install both `dependencies` and `devDependencies` but do not prune

You can get this behavior by setting the following env vars:
heroku config:set NPM_CONFIG_PRODUCTION=false YARN_PRODUCTION=false

`NODE_ENV != production`

If NODE_ENV is not set to production (ex: test or staging) then we will install devDependencies and skip pruning

edmorley · 2018-02-16T10:31:13Z

Would it be possible to provide an option to opt out of this? As-is this will mean we install/cache packages that we don't need.

It's such a shame that package.json doesn't have the concept of runtime vs buildtime vs dev/test dependencies, since the user confusion (and thus the need for this PR) really stems from buildtime deps having to be shoe-horned into either dependencies or devDependencies when really neither is always correct (think apps vs libraries).

jmorrell · 2018-02-16T16:03:23Z

Would it be possible to provide an option to opt out of this? As-is this will mean we install/cache packages that we don't need.

@edmorley I intend to make it so that you can configure it to only install dependencies or install both and skip pruning. I'll make sure to document that.

I believe this will be a better default for most users though.

hunterloftis · 2018-02-16T16:04:06Z

Agreed that it's a shame there's no concept of build dependencies (for things like build, but also CI).

While this will install devDependencies, based on the prune step @jmorrell mentioned above, I don't think it will cache them, so at least they won't bloat tarballs or increase transfer times. (I took the prune step to maybe be out-of-order as the last item, maybe that's wrong?)

jmorrell · 2018-02-16T16:06:58Z

@hunterloftis I'm currently pruning after the cache is saved. Otherwise the user needs to re-download all of the build tooling again the next time.

hunterloftis · 2018-02-16T16:23:57Z

Ah yeah, I hadn't thought about that. Glad you have :)

jmorrell · 2018-02-22T04:09:16Z

bin/compile

+  fi
+}
+
+header "Pruning devDependencies"


This is the terminology that npm uses, but I don't love it.

Is there a clearer way to get across "we're removing the modules defined in devDepenencies from node_modules"?

If it's the command in both yarn and npm, it's probably fine since that's what people will be familiar with.

jmorrell · 2018-02-22T20:58:19Z

@edmorley I can't tag you as a reviewer, but I'd appreciate it if you took a look :)

jmorrell · 2018-02-22T21:02:41Z

bin/compile

@@ -211,6 +211,17 @@ cache_build() {
 header "Caching build"


This changes a lot of the fixture files for tests which can be mostly ignored. The important changes are found in:

bin/compile
lib/dependencies.sh
lib/environment.sh
lib/failure.sh

With tests changes in:
test/run

jmorrell · 2018-02-22T21:03:31Z

lib/failure.sh

@@ -122,8 +122,7 @@ fail_yarn_outdated() {
  local log_file="$1"
  local yarn_engine=$(read_json "$BUILD_DIR/package.json" ".engines.yarn")

-  if grep -qi 'error: unknown option .--frozen-lockfile' "$log_file"; then
-    echo "ran"
+  if grep -qi 'error .install. has been replaced with .add. to add new dependencies' "$log_file"; then


This is changed because in these earlier versions adding the --production flag changes the error message that is displayed

jmorrell · 2018-02-22T21:04:46Z

test/run

-testNoEnvVars() {
-  env_dir=$(mktmpdir)
-  compile "stable-node" "$(mktmpdir)" $env_dir
-  assertCaptured "NPM_CONFIG_PRODUCTION=true"


We're no longer setting NPM_CONFIG_PRODUCTION by default

jmorrell · 2018-02-22T21:05:09Z

test/run

@@ -263,10 +270,6 @@ testWarnDevDeps() {
  assertCaptured "A module may be missing"
  assertNotCaptured "This module may be specified"
  assertCapturedError
-  compile "missing-devdeps-2"


This will no longer result in an error so I've removed it

jmorrell · 2018-02-22T21:09:02Z

test/run

@@ -712,7 +777,7 @@ testShrinkwrap() {
  compile "shrinkwrap"
  assertCaptured "[email protected]"
  assertCaptured "[email protected]"
-  assertNotCaptured "mocha"
+  assertCaptured "mocha@2.0.1"


This is now installed by default

jmorrell · 2018-02-22T21:09:57Z

test/run

@@ -811,6 +918,10 @@ compileTest() {
  for f in ${compile_dir}/.profile.d/*; do source $f > /dev/null 2> /dev/null ; done

  capture ${bp_dir}/bin/test ${compile_dir}
+


Fixes a bug in the compileTest function that would cause subsequent tests to fail

hone · 2018-02-27T23:38:43Z

lib/dependencies.sh

+yarn_prune_devdependencies() {
+  local build_dir=${1:-} 
+
+  if [ $NODE_ENV == "test" ]; then


This will be caught by the elif clause below. Do we want the explicit messaging here?

…ck-nodejs#519

hone · 2018-02-28T00:10:01Z

lib/dependencies.sh

+  local build_dir=${1:-} 
+  local npm_version=$(npm --version)
+
+  if [ $NODE_ENV == "test" ]; then


Same as the yarn comment above, but wanted to point it out for completeness sake.

hone · 2018-02-28T00:15:55Z

I ran through all the (non test) code. This looks good. I tried to find a OS X event library in node, but it looks like all file watcher libs are cross platform that are popular. I'm not sure of other platform specific libs that would only show up in node.

As a suggestion to provide helpful messaging to users who builds work now but may break with this change, the buildpack can mark in the cache the buildpack version number and the success state with this PR. If the build fails and the last build was not using this buildpack and the last build was not a success, then we can display how to opt into the old functionality.

mars

I've verified that the buildpack & reference apps that I maintain deploy cleanly using this dev-dependencies-change branch:

Thanks for your meticulous efforts on this @jmorrell 🎩

edmorley

Hi! I had a quick read through - looks great :-)
(Also glad there is an option for those that need to revert to the previous behaviour)

Things I noticed:

Does warn_missing_devdeps in failure.sh need updating to also check for YARN_PRODUCTION?
Can the NPM_CONFIG_PRODUCTION export now be removed from bin/test-compile?
In environment.sh's list_node_config, should the mcount "npm-config-production-true" also include YARN_PRODUCTION (or a new count be added)?

edmorley · 2018-02-28T01:08:35Z

lib/dependencies.sh

+  if [ $NODE_ENV == "test" ]; then
+    echo "Skipping because NODE_ENV is 'test'"
+    return 0
+  elif [ $NODE_ENV != "production" ]; then


It might be worth quoting the $NODE_ENV and the various other variables in conditionals, since otherwise spurious spaces can cause a too many arguments bash error and the wrong result.

eg:

$ NODE_ENV="foo bar"; if [ $NODE_ENV != "production" ]; then echo "not production"; else echo "production"; fi bash: [: too many arguments production

Fwiw shellcheck is able to find issues like this (the Python buildpack runs it on Travis):

In lib/dependencies.sh line 103: elif [ $NODE_ENV != "production" ]; then ^-- SC2086: Double quote to prevent globbing and word splitting.

(https://github.com/koalaman/shellcheck/wiki/SC2086)

cough shellcheck is the best

Chronial · 2018-03-01T22:03:07Z

Just a heads up: The silent removal of NPM_CONFIG_PRODUCTION silently broke our deploy process. We only actually build our code on npm install if that variable was set, so a version without any js code was deployed.

jmorrell · 2018-03-01T22:19:10Z

@Chronial I'm sorry to hear that 😓 Thank you for letting me know. I've tried to be as backward-compatible as I compatible as I could, but this is likely a breaking change for a small subset of users.

If you need to test for an env var, NODE_ENV=production is probably the one you want, or better, put the build into a heroku-postbuild script: https://devcenter.heroku.com/articles/nodejs-support#heroku-specific-build-steps

Chronial · 2018-03-01T22:21:21Z

put the build into a heroku-postbuild script: https://devcenter.heroku.com/articles/nodejs-support#heroku-specific-build-steps

Thanks, that's what we did. I think Heroku would benefit from a mechanism to notify its users about breaking changes like this before they happen.

tomreid76 · 2018-03-02T14:24:27Z

This also broke our deploys because we use private devDependencies. A heads up to paying customers would have been really nice instead of sending us into a panic and having to update environment variables across many applications.

Cryptophobia · 2018-03-02T15:52:57Z

Apart from the breaking changes, thank you for this much needed change to the default behaviour of the buildpack!

jmorrell · 2018-03-02T17:41:12Z

@Chronial @tomreid76 I don't disagree at all and could have done a better job here. It's a difficult balance because sending out mass emails can cause more stress and confusion than it alleviates. We saw this when sending out security notices and getting hundreds of support tickets from confused users.

Thanks for the feedback, and we'll review this to figure out how we can improve messaging next time.

jmorrell · 2018-03-02T17:42:24Z

@tomreid76 Can you elaborate on how private devDependencies were affected?

tomreid76 · 2018-03-02T23:25:58Z

Sure. We have a few private GitHub repos as devDependencies. Before, they were not installed by default when deploying (using node 6.11.x and npm 3.10.10). This is exactly how we wanted it. As of the update yesterday, Heroku started attempting install of those respective devDeps and things started blowing up because Heroku has no access.

One other thing to note is that we actually have a pre-deploy step wherein we generate a versioned build artifact in Travis CI, and so our deploy to Heroku is really just a deploy since everything needed (prod dependencies) is bundled.

Admittedly perhaps not the most common use case, but a "verified" artifact is important for reasons I won't get into. :)

In any case, problem is solved, and so not a big deal, just in the future would have appreciate an email or something... I understand that has consequences also though.

This also looks like a significant undertaking and I don't want to downplay those efforts or anything. Thanks for all your efforts in what up to yesterday has been a fairly good and seamless experience for me and others on my team.

edmorley · 2018-03-02T23:50:55Z

Perhaps for more significant changes a "change X is planned for release in N days" entry to https://devcenter.heroku.com/changelog (in addition to the one made post-release) would be visible but yet not as spammy as an email?

(Anyone who doesn't want to be surprised by changes should subscribe to that feed by RSS, IMO)

Chronial · 2018-03-03T11:59:07Z

@jmorrell a suggestion for a simple notification system: Open an issue in the github repo titled "Breaking Changes Notifications" and just add a comment there a few days before you deploy breaking changes. Everybody who is interested can just subscribe to that issue. Since it's opt-in this should not cause any confusion.

Then maybe reference that issue in this Repo's README and the heroku node.js docs.

darrenklein · 2018-03-07T23:23:28Z

@jmorrell I set the following env variables on my app

heroku config:set NPM_CONFIG_PRODUCTION=true YARN_PRODUCTION=true NODE_ENV=production

but Heroku is still trying to install my devDependencies which is breaking my deployment. What might I be missing?

jmorrell · 2018-03-08T00:29:34Z

@darrenklein That doesn't sound right. Can you open a support ticket at help.heroku.com and give me the ticket #? I'm happy to take a look

jjalan · 2018-03-08T08:28:08Z

@jmorrell - I have set up for my app

heroku config:set NPM_CONFIG_PRODUCTION=true YARN_PRODUCTION=true NODE_ENV=production

However, review apps created do not inherit these settings.

Is this a bug or design? If by design, how can we enable set NPM_CONFIG_PRODUCTION=true YARN_PRODUCTION=true for review apps?

jjalan · 2018-03-08T08:34:41Z

@jmorrell - I guess one way I could think about that is setting them to true in app.json env section.

darrenklein · 2018-03-08T16:23:42Z

@jmorrell I actually have a ticket open, which is how I was directed to this thread... it's ticket #560469.

Here's the error log from the breaking deployment - you can see that those env variables have been set, but the package that it's breaking on is from my devDependencies.

remote: Compressing source files... done.
remote: Building source:
remote: 
remote: -----> Node.js app detected
remote: 
remote: -----> Creating runtime environment
remote:        
remote:        NPM_CONFIG_LOGLEVEL=error
remote:        NPM_CONFIG_PRODUCTION=true
remote:        YARN_PRODUCTION=true
remote:        NODE_VERBOSE=false
remote:        NODE_ENV=production
remote:        NODE_MODULES_CACHE=true
remote: 
remote: -----> Installing binaries
remote:        engines.node (package.json):  8.9.4
remote:        engines.npm (package.json):   5.7.1
remote:        
remote:        Resolving node version 8.9.4...
remote:        Downloading and installing node 8.9.4...
remote:        Bootstrapping npm 5.7.1 (replacing 5.6.0)...
remote:        npm 5.7.1 installed
remote: 
remote: -----> Restoring cache
remote:        Skipping cache restore (new-signature)
remote: 
remote: -----> Building dependencies
remote:        Installing node modules (package.json + package-lock)
remote:        npm ERR! Error while executing:
remote:        npm ERR! /usr/bin/git ls-remote -h -t ssh://[email protected]/my-team/eslint-config-custom-config.git
remote:        npm ERR!
remote:        npm ERR! Host key verification failed.
remote:        npm ERR! fatal: Could not read from remote repository.
remote:        npm ERR!
remote:        npm ERR! Please make sure you have the correct access rights
remote:        npm ERR! and the repository exists.
remote:        npm ERR!
remote:        npm ERR! exited with error code: 128
remote:        
remote:        npm ERR! A complete log of this run can be found in:
remote:        npm ERR!     /tmp/npmcache.hhLmL/_logs/2018-03-07T23_09_52_031Z-debug.log
remote: 
remote: -----> Build failed
remote:        
remote:        We're sorry this build is failing! You can troubleshoot common issues here:
remote:        https://devcenter.heroku.com/articles/troubleshooting-node-deploys
remote:        
remote:        If you're stuck, please submit a ticket so we can help:
remote:        https://help.heroku.com/
remote:        
remote:        Love,
remote:        Heroku
remote:        
remote:  !     Push rejected, failed to compile Node.js app.
remote: 
remote:  !     Push failed
remote: Verifying deploy...
remote: 
remote: !	Push rejected to my-app-12345.
remote:

From the package-lock.json:

    "eslint-config-custom-config": {
      "version": "git+ssh://[email protected]/my-team/eslint-config-custom-config.git#859a39ac261140d442919f0f36dae5e0a2cf322a",
      "dev": true
    },

I also tried setting the buildpack to an earlier version, but it was still resulting in the same failure, which makes me think that something else may be up. Any help you can offer would be much appreciated.

mars · 2018-03-08T17:46:14Z

@darrenklein said,

remote:        npm ERR! Error while executing:
remote:        npm ERR! /usr/bin/git ls-remote -h -t ssh://[email protected]/my-team/eslint-config-custom-config.git
remote:        npm ERR!
remote:        npm ERR! Host key verification failed.
remote:        npm ERR! fatal: Could not read from remote repository.

How the Gitlab ssh authentication configured? Where is the private key? Does it come from another buildpack? If this is a Review App, is it missing a config var for auth?

jmorrell · 2018-03-08T18:36:27Z

@mars I'm answering him in a support ticket :)

tl;dr - When you have a private git repo in your package-lock.json, npm tries to resolve it when building out the dependency tree, even if it won't be installed. This would have been an issue before this change as well.

I'm not sure if this is a bug in npm or a limitation of its algorithm.

In this case it seems like moving the git dependency to optionalDependencies fixes the build without disrupting the local workflow.

Edit: Nope, I was wrong. npm forces you to add authorization when the git repo ends up in the lockfile regardless.

jmorrell requested a review from a team as a code owner February 16, 2018 01:54

jmorrell mentioned this pull request Feb 16, 2018

Add an option to prune packages after build #515

Closed

lirbank mentioned this pull request Feb 20, 2018

Consider installing devDependencies by default #431

Closed

Jeremy Morrell added 3 commits February 21, 2018 14:00

WIP

18e3b01

foobar

1fddd31

Make tests pass

30f5499

jmorrell force-pushed the dev-dependencies-change branch from db8ce6e to 30f5499 Compare February 22, 2018 03:02

Jeremy Morrell added 2 commits February 21, 2018 19:31

Add CI tests

6fbbaaf

Fix CI tests

92d5b9f

jmorrell commented Feb 22, 2018

View reviewed changes

jmorrell requested review from mars and hone February 22, 2018 06:59

jmorrell changed the title ~~[WIP] Install devDependencies by default~~ Install devDependencies by default Feb 22, 2018

jmorrell requested a review from hunterloftis February 22, 2018 20:16

Jeremy Morrell added 2 commits February 22, 2018 12:50

Add PREBUILD tests

cae423a

Add note to tests

751bf1b

jmorrell commented Feb 22, 2018

View reviewed changes

Add comments on test cases

74c3e87

hone reviewed Feb 27, 2018

View reviewed changes

mars added a commit to mars/create-react-app-buildpack that referenced this pull request Feb 28, 2018

Verify Node buildpack's dev-dependencies-change heroku/heroku-buildpa…

d55c472

…ck-nodejs#519

mars mentioned this pull request Feb 28, 2018

Verify Node buildpack's dev-dependencies-change mars/create-react-app-buildpack#90

Closed

hone reviewed Feb 28, 2018

View reviewed changes

hone approved these changes Feb 28, 2018

View reviewed changes

mars approved these changes Feb 28, 2018

View reviewed changes

edmorley reviewed Feb 28, 2018

View reviewed changes

Jeremy Morrell added 2 commits February 28, 2018 16:24

Quote variable

4c44116

Add changelog entry

bf4c75c

jmorrell merged commit bb9b5a9 into master Mar 1, 2018

edmorley mentioned this pull request Mar 1, 2018

Yarn rebuilds native dependencies during pruning #526

Open

jmorrell deleted the dev-dependencies-change branch March 2, 2018 04:49

caseywatts mentioned this pull request Aug 20, 2018

Update opn to latest release BrowserSync/browser-sync#1582

Merged

jmorrell mentioned this pull request Nov 4, 2018

Support "build" npm script #479

Closed

jmorrell mentioned this pull request Feb 20, 2019

Deploy with auto build mars/create-react-app-buildpack#156

Merged

sonicdoe mentioned this pull request Feb 28, 2021

Prune devDependencies after running npm install in production #259

Closed

		@@ -811,6 +918,10 @@ compileTest() {
		for f in ${compile_dir}/.profile.d/*; do source $f > /dev/null 2> /dev/null ; done

		capture ${bp_dir}/bin/test ${compile_dir}

Install devDependencies by default #519

Install devDependencies by default #519

Conversation

jmorrell commented Feb 16, 2018 • edited Loading

Only install dependencies, the current default behavior

Install both dependencies and devDependencies but do not prune

NODE_ENV != production

edmorley commented Feb 16, 2018 • edited Loading

jmorrell commented Feb 16, 2018

hunterloftis commented Feb 16, 2018 • edited Loading

jmorrell commented Feb 16, 2018

hunterloftis commented Feb 16, 2018

Choose a reason for hiding this comment

hone Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

jmorrell commented Feb 22, 2018

jmorrell Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hone Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hone commented Feb 28, 2018 • edited Loading

mars left a comment • edited Loading

Choose a reason for hiding this comment

edmorley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chronial commented Mar 1, 2018

jmorrell commented Mar 1, 2018

Chronial commented Mar 1, 2018

tomreid76 commented Mar 2, 2018

Cryptophobia commented Mar 2, 2018

jmorrell commented Mar 2, 2018

jmorrell commented Mar 2, 2018 • edited Loading

tomreid76 commented Mar 2, 2018

edmorley commented Mar 2, 2018 • edited Loading

Chronial commented Mar 3, 2018

darrenklein commented Mar 7, 2018

jmorrell commented Mar 8, 2018

jjalan commented Mar 8, 2018

jjalan commented Mar 8, 2018

darrenklein commented Mar 8, 2018

mars commented Mar 8, 2018

jmorrell commented Mar 8, 2018 • edited Loading

jmorrell commented Feb 16, 2018 •

edited

Loading

Only install `dependencies`, the current default behavior

Install both `dependencies` and `devDependencies` but do not prune

`NODE_ENV != production`

edmorley commented Feb 16, 2018 •

edited

Loading

hunterloftis commented Feb 16, 2018 •

edited

Loading

hone Feb 27, 2018 •

edited

Loading

jmorrell Feb 22, 2018 •

edited

Loading

hone Feb 27, 2018 •

edited

Loading

hone commented Feb 28, 2018 •

edited

Loading

mars left a comment •

edited

Loading

jmorrell commented Mar 2, 2018 •

edited

Loading

edmorley commented Mar 2, 2018 •

edited

Loading

jmorrell commented Mar 8, 2018 •

edited

Loading