Skip to content
This repository has been archived by the owner on Nov 16, 2019. It is now read-only.

Don't always include e.g. the history package #15

Merged
merged 1 commit into from
Nov 5, 2015
Merged

Don't always include e.g. the history package #15

merged 1 commit into from
Nov 5, 2015

Conversation

taion
Copy link
Contributor

@taion taion commented Oct 8, 2015

Fixes npm/npm#9642

@taion
Copy link
Contributor Author

taion commented Oct 8, 2015

Oops, hold on.

@taion
Copy link
Contributor Author

taion commented Oct 8, 2015

Fixed. I flipped the conditional on accident. I improved the test to also catch that.

For reasons unclear to me, cleanup fails with rimraf.sync, so I switched to using async rimraf there.

@tamzinblake
Copy link

@othiym23 This would be super-helpful

@othiym23 othiym23 self-assigned this Oct 30, 2015
@othiym23
Copy link
Contributor

It's got a test, the problem is real, and I'll try to get this merged and released shortly! Thanks for the nudge, @thomblake!

@othiym23 othiym23 merged commit 0458457 into npm:master Nov 5, 2015
@taion taion deleted the hardcoded-ignores branch November 5, 2015 01:14
@taion
Copy link
Contributor Author

taion commented Nov 5, 2015

Thanks!

@othiym23
Copy link
Contributor

othiym23 commented Nov 5, 2015

Included in [email protected], which will be landed in npm shortly. Thank you!

@othiym23
Copy link
Contributor

othiym23 commented Nov 5, 2015

Landed in npm as npm/npm@66abc60 in release-3.4.0, and npm/npm@3308daf in npm-2.14.10.

othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
No longer mandatorily include modules with names that match the npm file
whitelist (i.e. test to see if they're actually directories first).

Credit: @taion
PR-URL: npm/fstream-npm#15
othiym23 added a commit to npm/npm that referenced this pull request Nov 5, 2015
No longer mandatorily include modules with names that match the npm file
whitelist (i.e. test to see if they're actually directories first).

Credit: @taion
PR-URL: npm/fstream-npm#15
@tamzinblake
Copy link

Awesome!

@timaschew
Copy link

still broken with npm 2.14.10

$ npm --version
2.14.10

$ cat package.json
{
  "name": "test1",
  "version": "1.0.0",
  "main": "index.js",
  "dependencies": {
    "redux-router": "^1.0.0-beta3"
  }
}

$ npm i
npm WARN package.json [email protected] No description
npm WARN package.json [email protected] No repository field.
npm WARN package.json [email protected] No README data
npm WARN package.json [email protected] No license field.
[email protected] node_modules/redux-router
└── [email protected]

$ npm ls
[email protected] /Users/awilhelm/dev/test1
└─┬ [email protected]
  ├── [email protected]
  └── [email protected] extraneous

npm ERR! extraneous: [email protected] /Users/awilhelm/dev/test1/node_modules/redux-router/node_modules/history

$ npm shrinkwrap
npm ERR! Darwin 15.0.0
npm ERR! argv "/usr/local/bin/iojs" "/usr/local/share/npm/bin/npm" "shrinkwrap"
npm ERR! node v4.1.2
npm ERR! npm  v2.14.10

npm ERR! Problems were encountered
npm ERR! Please correct and try again.
npm ERR! extraneous: [email protected] /Users/awilhelm/dev/test1/node_modules/redux-router/node_modules/history
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/awilhelm/dev/test1/npm-debug.log

@taion
Copy link
Contributor Author

taion commented Nov 9, 2015

This was a fix for packagers, not for users.

@timaschew
Copy link

So the module needs to be published again?
Which one, history itself or all modules which rely on history?

@taion
Copy link
Contributor Author

taion commented Nov 9, 2015

Every module that depends on history, yes. The issue was that the packed tarballs included history - that directory doesn't get expunged from the tarballs already released to npm just because we've patched fstream-npm.

@zhiyelee
Copy link

zhiyelee commented Dec 1, 2015

Awesome thanks!

@taion
Copy link
Contributor Author

taion commented Dec 30, 2015

@othiym23 It looks like the equivalent file is vendored for npm3 per https://github.com/npm/npm/blob/v3.5.2/lib/utils/tar.js#L64, so this bug is actually not fixed for npm3 users. How can we go about getting this resolved?

@lukekarrys
Copy link

@taion I created a PR to fix it for npm@3 but never found the time to go back and add tests to the PR npm/npm#10445 From the comments over there, it sounds like once tests get added it can be merged.

@taion
Copy link
Contributor Author

taion commented Dec 30, 2015

@lukekarrys Thanks!

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

Successfully merging this pull request may close these issues.

6 participants