cache: add-local: do full "npm install" w/ prepublish prior to packing #8725

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@iarna
Member
iarna commented Jun 27, 2015

Fixes: #7040 #3055

@iarna iarna added the in progress label Jun 27, 2015
@iarna iarna modified the milestone: 3.0.1, 3.2.0, 3.1.0 Jun 29, 2015
@iarna iarna cache: add-local: do full "npm install" w/ prepublish prior to packing
Fixes: #7040
2166afc
@iarna
Member
iarna commented Jul 1, 2015

See also #7627

@boneskull
Contributor

👍

@iarna iarna commented on the diff Jul 7, 2015
lib/cache/add-local.js
@@ -92,13 +93,21 @@ function addLocalDirectory (p, pkgData, shasum, cb) {
getCacheStat(function (er, cs) {
mkdir(path.dirname(pj), function (er, made) {
if (er) return cb(er)
- var doPrePublish = !pathIsInside(p, npm.tmp)
- if (doPrePublish) {
- lifecycle(data, 'prepublish', p, iferr(cb, thenPack))
- } else {
- thenPack()
+ var isProduction = npm.config.get('production')
+ var dir = npm.dir
+ var prefix = npm.prefix
+
+ npm.config.set('production', false)
@iarna
iarna Jul 7, 2015 Member

This stuff, in particular, is extremely scary and probably shouldn't be allowed in production code.

@iarna
iarna Jul 8, 2015 Member

By "this stuff", I mean, trying to get/set things from the global config object, when who knows what else is running and could be effected by this. To land this the production flag needs to be isolated to the installer object, so that we can instantiate an object here with the config we need and run that, rather than calling install.

@iarna iarna modified the milestone: 3.x, 3.2.0 Jul 8, 2015
@othiym23 othiym23 added the git label Jul 16, 2015
@iarna
Member
iarna commented Sep 16, 2015

Adopting this is blocked by § 3.7 of the roadmap

@iarna
Member
iarna commented Oct 12, 2015

Closing this, we'll repull when § 3.7 of the roadmap is resolved.

@iarna iarna closed this Oct 12, 2015
@iarna iarna removed the in progress label Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment