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

added npm-shrinkwrap.json support #200

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

luscus
Copy link

@luscus luscus commented Oct 9, 2015

Hello,

I'm making heavy use of npm shrinkwrap and I wanted to enable the tool to take it into account - some of my packages have dependencies defined with */latest to always be up to date on npm update.
The used version is then pinned down automatcally on release by gulp-shrinkwrap.

Problem was that David wasn't showing the correct information because only interpreting the *version, over time it would not have shown deprecated dependencies...

The change merges the information contained in the npm-shrinkwrap.json with the package.json before the normal processing occurs. It overwrites the Manifest versions with the pinned versions: */latest becomes x.y.z

Due to the new depth level in the code, the diff is not showing useful data: I have commented the real new lines => one block is relevant

@@ -50,7 +50,7 @@ exports.getManifest = function (user, repo, ref, authToken, cb) {

batch.push(batchKey, cb)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new block BEGIN

})
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new code block END

@luscus
Copy link
Author

luscus commented Nov 5, 2015

Could I get a feedback on this pull?

@alanshaw
Copy link
Owner

Hi @luscus,

Thanks for your contribution. It's an interesting issue. May I ask why you're using "*" or "latest" in your package.json and not a more usual "^" or "~" version range?

My concern with the change is that whilst this works for your workflow, I have no idea if this is something that all shrinkwrap users want, or expect from david.dm.org.

I'd be more open to shrinkwrap support like this through a querystring parameter.

Let me know what you think.

@luscus
Copy link
Author

luscus commented Nov 12, 2015

Hello @alanshaw,

we had bad experience in the past with stable services, the longer they're running the more probability they would break because of some deep dependency (dependency from dependency).
Our package.json always use full version numbers, but our dependencies mostly use wildcards (~, ^,...) and some of those projects do not use SEMVER correctly: over time the break chance is growing.

In order to prevent this and to be able to check out and run services without problems, we have been using npm-shrinkwrap extensively. It freeze all version numbers in deep for all used packages.
npm shrinkwrap is run per git-hook on pre-push

npm install uses the npm-shrinkwrap.json file to request the right versions: no more breaks

As to keep our dependencies up to date, we use git-hooks to update them on post-checkout or post-merge.
npm update ignores the shrinkwrap file and uses the package.json versions: "latest" always return the up to date version. For stable packages that rarely need maintenance this enforce the dependency updating.

I know there is an argument about shrinkwrap freezing also the source of the package (npmjs.org or some corporate repo) with the "from" property. We beleave shrinkwrap is part of the dev workflow to ensure all time running versions, as such it should only freeze the version - not the source - and we workaround the "from" property using gulp-shrinkwrap (which delete this property after the file was created)

For myself my expectations about david.dm.org is to provide "true" dependency information: what I get with an npm install
It should reflect the gap between the used version and the latest available - anything else would be no information.

I'm open to query string parameters and would implement it: shrimkwrap=true ?

@alanshaw
Copy link
Owner

Thanks for the info, I understand the reasons for using shrinkwrap but what I was asking was why use "latest" or "*" in your package.json. When you npm update you're going to get the latest versions...true, but you'll potentially bring in breaking changes which could be a nightmare to solve if you're not careful to update just a single dependency at a time. Personally I have my version ranges in package.json set using ^ or ~ even if using shrinkwrap, so when I npm update I (optimistically) don't get breaking changes. Major version changes I examine (after consulting david-dm.org) and update carefully.

Anyway, yes, I still stand behind my original concern that the proposed behaviour is not something all shinrkwrap users want. However, I appreciate inspecting the shrinkwrap dependencies is something developers might want to do. I've worked in a few places where the shrinkwrap deps have drifted from the versions defined in package.json so can see why you might want to do that! I think the querystring shrinkwrap=true is a good way to expose the functionality your proposing in a backwards compatible way.

@luscus
Copy link
Author

luscus commented Nov 20, 2015

Great! I will implement it in the coming days and submit it for review

I never had that much problems with "latest", I use a minimalistic approach to dependencies.
The behaviour is intended in order to use the patches or changes - the exception is setting a number if somehow the new version can't be integrated (never append to me yet)

@gabrielcsapo
Copy link
Collaborator

@luscus would you be able to rebase this?

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

Successfully merging this pull request may close these issues.

3 participants