install: correct reqBy spec for opt deps

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.

[larsgw]

Return the correct spec for optional dependencies in getRequested.

See https://npm.community/t/242

[larsgw]

As with #115:

I'm not really sure how to integration-test this without actually cloning git projects at the moment.

Ideas are welcome.

[aeschright]

This patch won't resolve the underlying problem -- we actually need to fix what's being put into the package-lock.json so that optional git dependencies are stored using the same format as the others.

[larsgw]

If I remember correctly, that's what this patch is supposed to do (I'll check this afternoon). Or are you suggesting a more general fix?

[larsgw]

I checked, given this package.json:

{
"name": "060",
"version": "1.0.0",
"optionalDependencies": {
"left-pad": "git+https://github.com/jeffora/left-pad.git#custom-left-pad"
}
}

npm@6.5.0 produces this entry for left-pad:

..."left-pad": {
"version": "1.3.0",
"resolved": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"optional": true
}
...

While my patch produces this:

..."left-pad": {
"version": "git+https://github.com/jeffora/left-pad.git#806c9eda55a3ac4ad365a344ac9024c3ea183f8c",
"from": "git+https://github.com/jeffora/left-pad.git#custom-left-pad",
"optional": true
}
...

The latter works with npm ci, but I can add back the resolved field. However, that's not how dependencies and devDependencies work.

I'll try to add tests (given #115 (comment)) later, but not before my thoughts are confirmed of course.

[iarna]

We DO want output like what you quoted, but I'm concerned about your patch for this reason. You wrote this:

return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) return npa.resolve(name, deps[name] || devDeps[name] || optDeps[name], reqBy.realpath)

And the thing is, because normalize-package-data (and in turn read-package-json) add all optional deps to regular deps, this patch should be a no-op, because the package metadata we get out of read-package-json will always have all optional deps also included in dependencies.

[iarna]

If your patch does fix something, it means something else has gone very wrong somewhere along the way, and we should patch that.

[gurpreetatwal]

Hi all, so I stepped through the code and I believe I have figured out where along the way things went bad. Specifically, it's in the savePackageJson function in install/lib/save.js.

cli/lib/install/save.js

Lines 63 to 70 in ab0f026

	fs.readFile(saveTarget,'utf8',iferr(next,function(packagejson){
	constindent=detectIndent(packagejson).indent
	constnewline=detectNewline(packagejson)
	try{
	tree.package=parseJSON(packagejson)
	}catch(ex){
	returnnext(ex)
	}

It replaces tree.package as read by read-package-json with just a fs.readFile of package.json. The in turn breaks that invariant that all optional dependencies should be included in dependencies.

Also the next function eventually invokes saveShrinkWrap as called by exports.saveRequested in lib/install/save.js

I'm not sure what the fix is here as I don't understand the npm code too well, but willing to offer more help if I can get some pointers.

[larsgw]

It seems the new tree.package from parseJSON isn't passed to anywhere else (apart from saveShrinkwrap of course), so I think it's safe to do this:

letpkgtry{pkg=parseJSON(packagejson)}catch(ex){returnnext(ex)}

As for the tests, I'm not really sure.

[DrSensor]

Hi, I also bump a similar issue but with file:

package-lock.json

{
..."@obot/cli": {
"version": "0.2.1",
"resolved": "/home/wildan/Projects/OSS/bot-byte/packages/cli",
"optional": true,
"requires": {
"@oclif/command": "^1.5.8",
"@oclif/config": "^1.12.0",
"@oclif/errors": "^1.2.2",
"@oclif/plugin-help": "^2.1.4"
},
"dependencies": {...}
},
...
}

package.json

{
..."optionalDependencies": {
"@obot/cli": "file:packages/cli"
}
...
}

$ npm cinpm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.npm ERR!npm ERR!npm ERR! Invalid: lock file's @obot/cli@0.2.1 does not satisfy @obot/cli@file:packages/clinpm ERR!npm ERR! A complete log of this run can be found in:npm ERR! /home/wildan/.npm/_logs/2019-01-29T09_06_29_704Z-debug.log

[gurpreetatwal]

@zkat@iarna any thoughts on the above solution? I'd love to help implement, just need some guidance. This same fix also fixes another issue my team is experiencing with package-lock.json and github dependencies. I'm still trying to create a minimal reproduction of that problem, but the code above seems troublesome

[larsgw]

How can I help you?

BTW, another possible solution: save package-lock.json before saving package.json.

[nictownsend]

Are there any updates on this? I have optional dependencies that can't currently be pushed to a registry.