Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Installed devDependencies with --prod flag set #3577

Closed
ghost opened this issue Jun 5, 2017 · 23 comments
Closed

Installed devDependencies with --prod flag set #3577

ghost opened this issue Jun 5, 2017 · 23 comments
Labels

Comments

@ghost
Copy link

ghost commented Jun 5, 2017

This is an extension of #3473, which was closed during issue pruning. This is not the same issue.

Do you want to request a feature or report a bug?
It's a bug.

What is the current behavior?
Installs all package development dependencies when --prod flag set.

If the current behavior is a bug, please provide the steps to reproduce.

yarn add fetch-inject --prod && (cd node_modules && tree -h)

What is the expected behavior?
Only following output is printed to the output buffer when tree is run with -h flag:

fetch-inject
.
└── [ 442]  fetch-injects
    ├── [ 758]  LICENSE
    ├── [ 12K]  README.md
    ├── [ 466]  bower.json
    ├── [ 272]  dist
    │   ├── [2.5K]  fetch-inject.es.js
    │   ├── [ 868]  fetch-inject.es.min.js
    │   ├── [2.5K]  fetch-inject.js
    │   ├── [ 870]  fetch-inject.min.js
    │   ├── [2.7K]  fetch-inject.umd.js
    │   └── [1012]  fetch-inject.umd.min.js
    ├── [ 136]  docs
    │   ├── [169K]  fetch-inject-serviceworker-caching.png
    │   └── [178K]  fetch-inject-unprimed-cache.png
    ├── [1.3K]  package.json
    ├── [1.0K]  rollup.config.js
    ├── [ 136]  src
    │   ├── [1.5K]  fetch-inject.js
    │   └── [1.0K]  injectors.js
    └── [ 102]  test

5 directories, 15 files

Please mention your node.js, yarn and operating system version.

node -v && yarn -v && system_profiler SPSoftwareDataType
v6.3.1
yarn install v0.24.4
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
✨  Done in 10.63s.
Software:

    System Software Overview:

      System Version: macOS 10.12.4 (16E195)
      Kernel Version: Darwin 16.5.0
      Boot Volume: Macintosh HD
      Boot Mode: Normal
      Computer Name: Josh’s MacBook Pro
      User Name: Josh Habdas (jhabdas)
      Secure Virtual Memory: Enabled
      System Integrity Protection: Enabled
      Time since boot: 26 days 1 minute
@rally25rs
Copy link
Contributor

rally25rs commented Jun 5, 2017

(Edit: brew installed tree and re-ran command to get comparable output)

I'm unable to reproduce this on osx sierra, yarn 0.24.4

(mkdir and cd into a new/clean directory)

$ yarn init -y
yarn init v0.24.4
warning The yes flag has been set. This will automatically answer yes to all questions which may have security implications.
success Saved package.json
✨  Done in 0.03s.

$ yarn add fetch-inject --prod && (cd node_modules && tree -h)
yarn add v0.24.4
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
└─ fetch-inject@1.7.2
✨  Done in 0.34s.
.
└── [ 442]  fetch-inject
    ├── [ 758]  LICENSE
    ├── [ 12K]  README.md
    ├── [ 466]  bower.json
    ├── [ 272]  dist
    │   ├── [2.5K]  fetch-inject.es.js
    │   ├── [ 868]  fetch-inject.es.min.js
    │   ├── [2.5K]  fetch-inject.js
    │   ├── [ 870]  fetch-inject.min.js
    │   ├── [2.7K]  fetch-inject.umd.js
    │   └── [1012]  fetch-inject.umd.min.js
    ├── [ 136]  docs
    │   ├── [169K]  fetch-inject-serviceworker-caching.png
    │   └── [178K]  fetch-inject-unprimed-cache.png
    ├── [1.3K]  package.json
    ├── [1.0K]  rollup.config.js
    ├── [ 136]  src
    │   ├── [1.5K]  fetch-inject.js
    │   └── [1.0K]  injectors.js
    └── [ 102]  test

5 directories, 15 files

$ yarn list
yarn list v0.24.4
└─ fetch-inject@1.7.2
✨  Done in 0.10s.

$ ls node_modules/
fetch-inject

Is it possible that you had previously added fetch-inject as a dev dependency? What does your package.json and yarn.lock file contain?


Side note; I'm not sure the prod flag even matters. The default is to make it a regular dependency which shouldn't add the devDependencies anyway.

$ yarn add fetch-inject && (cd node_modules && tree -h)
yarn add v0.24.4
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
└─ fetch-inject@1.7.2
✨  Done in 0.34s.
.
└── [ 442]  fetch-inject
    ├── [ 758]  LICENSE
    ├── [ 12K]  README.md
    ├── [ 466]  bower.json
    ├── [ 272]  dist
    │   ├── [2.5K]  fetch-inject.es.js
    │   ├── [ 868]  fetch-inject.es.min.js
    │   ├── [2.5K]  fetch-inject.js
    │   ├── [ 870]  fetch-inject.min.js
    │   ├── [2.7K]  fetch-inject.umd.js
    │   └── [1012]  fetch-inject.umd.min.js
    ├── [ 136]  docs
    │   ├── [169K]  fetch-inject-serviceworker-caching.png
    │   └── [178K]  fetch-inject-unprimed-cache.png
    ├── [1.3K]  package.json
    ├── [1.0K]  rollup.config.js
    ├── [ 136]  src
    │   ├── [1.5K]  fetch-inject.js
    │   └── [1.0K]  injectors.js
    └── [ 102]  test

5 directories, 15 files

There must be some other factor at play here...

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Thanks for taking a look @rally25rs. I'm doing a little more digging on my end to provide a better minimal test case for reproduction.

@ghost ghost changed the title Downloads unexpected dependencies Downloads unexpected dependencies without existing yarn lock Jun 5, 2017
@ghost
Copy link
Author

ghost commented Jun 5, 2017

@rally25rs I noticed your repro steps did not match my own. Here's a package.json file. Please try again without the yarn init -y (which I assume creates an initial lock):

{
  "name": "abracadabra-bee",
  "version": "0.0.0-development",
  "description": "Fully, man.",
  "module": "dist/abracadabra.es.js",
  "main": "dist/abracadabra.umd.js",
  "repository": "abracadabra/bee",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "devDependencies": {
    "commitizen": "^2.9.6",
    "cz-conventional-changelog": "^2.0.0",
    "husky": "^0.13.4",
    "rimraf": "^2.6.1",
    "rollup": "^0.42.0",
    "rollup-plugin-license": "^0.4.0",
    "rollup-plugin-uglify": "^2.0.1",
    "semantic-release": "^6.3.2",
    "standard": "^9.0.2",
    "uglify-es": "^3.0.12"
  },
  "standard": {
    "ignore": [
      "/dist"
    ]
  },
  "license": "ISC"
}

@ghost
Copy link
Author

ghost commented Jun 5, 2017

https://yarnpkg.com/en/docs/migrating-from-npm

Pulling doc for reference:

When you run either yarn or yarn add <package>, Yarn will generate a yarn.lock file within the root directory of your package. You don’t need to read or understand this file - just check it into source control.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

And here's a gist containing the yarn.lock produced after following riffing on the NPM migration instructions using the --prod flag: https://gist.github.com/jhabdas/a09a019ff5e3e41e3cb4e61cc70a3e87

@ghost ghost changed the title Downloads unexpected dependencies without existing yarn lock Downloads unexpected dependencies Jun 5, 2017
@ghost
Copy link
Author

ghost commented Jun 5, 2017

The crux of this issue seems to be --prod flag is not respected when no yarn.lock is present, resulting in the download of unexpected development dependencies.

I raised #3473 two weeks ago as I was attempting to update my project README to add a yarn install command in addition to the following commands:

  • Get it on NPM with npm i -p fetch-inject
  • Bower with bower i -p fetch-inject

The intention here is to highlight the lib may be used without installation of any other dependencies, which I guess I'm struggling with as a result of this issue.

@rally25rs
Copy link
Contributor

You know what, I'm thinking the --production flag might just be broken.

yarn install --production seems to install all the devDependencies

This might be related to #3439 and #2304

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Thanks for the quick follow-up, @rally25rs. Updating title to reflect the true nature of this issue now that it seems we've nailed it and because the related issues are currently closed.

@ghost ghost changed the title Downloads unexpected dependencies Installed devDependencies with --prod flag set Jun 5, 2017
@rally25rs
Copy link
Contributor

Tested in 0.24.4 and 0.24.6. Both had the same behavior.

yarn install --production installs devDependencies listed in package.json

$ cat package.json
{
  "name": "yarn-test",
  "version": "0.0.0-development",
  "description": "yarn test",
  "devDependencies": {
    "rimraf": "^2.6.1"
  },
  "license": "ISC"
}

$ yarn install --production
yarn install v0.24.6
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
✨  Done in 0.52s.

$ yarn list
yarn list v0.24.6
├─ balanced-match@0.4.2
├─ brace-expansion@1.1.7
│  ├─ balanced-match@^0.4.1
│  └─ concat-map@0.0.1
├─ concat-map@0.0.1
├─ fs.realpath@1.0.0
├─ glob@7.1.2
│  ├─ fs.realpath@^1.0.0
│  ├─ inflight@^1.0.4
│  ├─ inherits@2
│  ├─ minimatch@^3.0.4
│  ├─ once@^1.3.0
│  └─ path-is-absolute@^1.0.0
├─ inflight@1.0.6
│  ├─ once@^1.3.0
│  └─ wrappy@1
├─ inherits@2.0.3
├─ minimatch@3.0.4
│  └─ brace-expansion@^1.1.7
├─ once@1.4.0
│  └─ wrappy@1
├─ path-is-absolute@1.0.1
├─ rimraf@2.6.1
│  └─ glob@^7.0.5
└─ wrappy@1.0.2
✨  Done in 0.12s.

$ yarn why rimraf
yarn why v0.24.6
[1/4] 🤔  Why do we have the module "rimraf"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
info Has been hoisted to "rimraf"
info This module exists because it's specified in "devDependencies".
✨  Done in 0.12s.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Did a quick pass through some of the source and issues and found:

Related issues:
#2739, #2819

Duplicate issues:
#3312, #3525, #3546, #3439, #3468, #3323, #2944, #3263, #2304, #2703

I gave up after that. Here's a search to turn up the rest.

Saw here a duplicate has been closed as a non-issue with justification: #3468 (comment)

Using my inbuilt cyclomatic complexity detector (that's a joke) I'd wager the bug somewhere in here:

yarn/src/config.js

Lines 289 to 300 in 4463175

if (opts.production === 'false') {
this.production = false;
} else if (
this.getOption('production') ||
(process.env.NODE_ENV === 'production' &&
process.env.NPM_CONFIG_PRODUCTION !== 'false' &&
process.env.YARN_PRODUCTION !== 'false')
) {
this.production = true;
} else {
this.production = !!opts.production;
}

And these lines stuck me as odd:

# Change this to "yarn install --production" once #1115 is fixed
yarn install --production

Wasn't able to determine if there was test coverage on the config (moved onto other tasks).

@rally25rs
Copy link
Contributor

rally25rs commented Jun 5, 2017

You know what, I just played with this some more, and realized some interesting behavior...

The yarn.lock file still lists the dev deps, as does yarn list and yarn why but the files for them are not actually extracted to node_modules/

$ cat package.json
{
  "name": "yarn-test",
  "version": "0.0.0-development",
  "description": "yarn test",
  "devDependencies": {
    "rimraf": "^2.6.1"
  },
  "license": "ISC"
}

$ rm -rf node_modules/

$ rm yarn.lock

$ yarn install --prod
yarn install v0.24.6
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
Done in 0.55s.

$ yarn list
yarn list v0.24.6
├─ balanced-match@0.4.2
├─ brace-expansion@1.1.7
│  ├─ balanced-match@^0.4.1
│  └─ concat-map@0.0.1
├─ concat-map@0.0.1
├─ fs.realpath@1.0.0
├─ glob@7.1.2
│  ├─ fs.realpath@^1.0.0
│  ├─ inflight@^1.0.4
│  ├─ inherits@2
│  ├─ minimatch@^3.0.4
│  ├─ once@^1.3.0
│  └─ path-is-absolute@^1.0.0
├─ inflight@1.0.6
│  ├─ once@^1.3.0
│  └─ wrappy@1
├─ inherits@2.0.3
├─ minimatch@3.0.4
│  └─ brace-expansion@^1.1.7
├─ once@1.4.0
│  └─ wrappy@1
├─ path-is-absolute@1.0.1
├─ rimraf@2.6.1
│  └─ glob@^7.0.5
└─ wrappy@1.0.2
✨  Done in 0.09s.

$ tree node_modules/
node_modules/

0 directories, 0 files

So now I'm questioning my above comment where I was thinking that devDependencies were being installed. I was relying on yarn list to tell me what was installed, but it looks like it actually reports what was resolved.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Scientific method suggests we shouldn't introduce any variables between steps until a new hypothesis can be drawn. To reproduce the issue grab the package.json I left in the above gist, replicate the environment, which uses a Node LTS, and follow the repro steps using the --prod flag and you should arrive at: https://pastebin.com/J68f8Hpy

Actual: 206 directories, 1909 files
Expected: 5 directories, 15 files

@rally25rs
Copy link
Contributor

rally25rs commented Jun 5, 2017

Seems to be dependent on what specific packages are in your devDependencies. Some (rimraf) will not have any deps installed after an install --prod.

Cut your package.json devDeps down to just this one:

  "devDependencies": {
    "standard": "^9.0.2"
  },

After a yarn install --prod:

$ ls node_modules
estraverse              is-fullwidth-code-point       number-is-nan

So it doesn't actually install your devDependencies, but some transient deps somehow get included, but not all of them either. I haven't quite nailed down the commonality in the 3 deps above that cause them to get installed. They all seem to come from different places in the dependency graph.

For example:

$ yarn why number-is-nan
yarn why v0.24.6
[1/4] 🤔  Why do we have the module "number-is-nan"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
info This module exists because "standard#eslint#inquirer#readline2#is-fullwidth-code-point" depends on it.

yet, eslint, inquirer, readline2 all aren't installed into node_modules.

If you look at readline2's dependencies:

  "dependencies": {
    "code-point-at": "^1.0.0",
    "is-fullwidth-code-point": "^1.0.0",
    "mute-stream": "0.0.5"
  },

code-point-at and mute-steam weren't installed, but is-fullwidth-code-point was. Very weird!

@rally25rs
Copy link
Contributor

OK, It looks like this is already fixed in master, which is actually why the previously mentioned issue was marked closed...

$ node ../yarn/bin/yarn.js add fetch-inject --prod && (cd node_modules && tree -h)
yarn add v0.26.0-0
info No lockfile found.
[1/4] Resolving packages...
warning semantic-release > npmconf@2.1.2: this package has been reintegrated into npm and is now out of date with respect to npm
warning semantic-release > @semantic-release/commit-analyzer > conventional-changelog@0.0.17: Please update conventional-changelog to >1.0.0. If you are running the cli, use conventional-changelog-cli
warning semantic-release > @semantic-release/condition-travis > travis-deploy-once > travis-ci > request > node-uuid@1.4.8: Use uuid module instead
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
└─ fetch-inject@1.7.2
Done in 3.48s.
.
└── [ 442]  fetch-inject
    ├── [ 758]  LICENSE
    ├── [ 12K]  README.md
    ├── [ 466]  bower.json
    ├── [ 272]  dist
    │   ├── [2.5K]  fetch-inject.es.js
    │   ├── [ 868]  fetch-inject.es.min.js
    │   ├── [2.5K]  fetch-inject.js
    │   ├── [ 870]  fetch-inject.min.js
    │   ├── [2.7K]  fetch-inject.umd.js
    │   └── [1012]  fetch-inject.umd.min.js
    ├── [ 136]  docs
    │   ├── [169K]  fetch-inject-serviceworker-caching.png
    │   └── [178K]  fetch-inject-unprimed-cache.png
    ├── [1.3K]  package.json
    ├── [1.0K]  rollup.config.js
    ├── [ 136]  src
    │   ├── [1.5K]  fetch-inject.js
    │   └── [1.0K]  injectors.js
    └── [ 102]  test

5 directories, 15 files

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Can we leave this one open until the release is dist tagged this time?

EDIT: Furthermore this is a common regression seems like based on the number of related or duplicative issues. Some additional investigation into root case and improved test coverage is advised.

Lastly, it would be stellar to update the CLI to use common behaviors such as allowing flags to be ordered before subcommands and to gain more parity with the NPM and Bower CLI for use of things like -p for --production and -v for displaying version info. But that's another issue.

@arcanis
Copy link
Member

arcanis commented Jun 6, 2017

The yarn.lock file still lists the dev deps, as does yarn list and yarn why but the files for them are not actually extracted to node_modules/

My guess is that this is because we always resolve all modules (including development modules) even when running in production mode, because otherwise the dependency tree could end up very different between production and development modes. We don't actually install dependencies that are not relevant to the current environment (or at least we shouldn't), but we still need to resolve them somehow :)

@bestander
Copy link
Member

As it is fixed in master we plan to release 0.27.0 early next week.

@ghost
Copy link
Author

ghost commented Jun 21, 2017

Still jumping the gun a little IMO. But I'll settle for a promise.

@arcanis
Copy link
Member

arcanis commented Jun 23, 2017

The 0.27.0 has been released! @JHabdas can you check that it fixes your use case?

@ghost
Copy link
Author

ghost commented Jun 23, 2017

On it

@ghost
Copy link
Author

ghost commented Jun 23, 2017

That's what I wanted to see:

.
└── [ 442]  fetch-inject
    ├── [ 758]  LICENSE
    ├── [ 12K]  README.md
    ├── [ 466]  bower.json
    ├── [ 272]  dist
    │   ├── [2.6K]  fetch-inject.es.js
    │   ├── [ 876]  fetch-inject.es.min.js
    │   ├── [2.7K]  fetch-inject.js
    │   ├── [ 878]  fetch-inject.min.js
    │   ├── [2.9K]  fetch-inject.umd.js
    │   └── [1020]  fetch-inject.umd.min.js
    ├── [ 136]  docs
    │   ├── [169K]  fetch-inject-serviceworker-caching.png
    │   └── [178K]  fetch-inject-unprimed-cache.png
    ├── [1.3K]  package.json
    ├── [1.0K]  rollup.config.js
    ├── [ 136]  src
    │   ├── [1.7K]  fetch-inject.js
    │   └── [1.0K]  injectors.js
    └── [ 102]  test

This issue may be closed now (half joking there, half not). By the way, I speculate NPM is about to become a general purpose front-end package manager as a result of the Bower deprecation notice. I hope you guys are able to capitalize on this opportunity quickly by introducing a pattern to define the split. Cheerio!

@pichouk
Copy link
Contributor

pichouk commented Jun 23, 2017

Just test it and it seems to work.

kyane@kyane-pc:/tmp/test$ cat package.json 
{
  "name": "test",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "lodash": "^4.17.4"
  },
  "devDependencies": {
    "test": "^0.6.0"
  }
}
kyane@kyane-pc:/tmp/test$ yarn install --production
yarn install v0.27.0
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 0.25s.
kyane@kyane-pc:/tmp/test$ ls -la node_modules/
total 32
drwxr-xr-x 3 kyane kyane  4096 Jun 23 18:09 .
drwxr-xr-x 3 kyane kyane  4096 Jun 23 18:09 ..
drwxr-xr-x 3 kyane kyane 20480 Jun 23 18:09 lodash
-rw-r--r-- 1 kyane kyane   581 Jun 23 18:09 .yarn-integrity

@ajhool
Copy link

ajhool commented Aug 29, 2018

I am potentially having this issue when using yarn workspaces and local file package references

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants