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

feat: add support for npm-shrinkwrap.json #185

Merged
merged 3 commits into from
Jun 4, 2017

Conversation

noreiller
Copy link
Contributor

Hi, I use your module since a few weeks, and as I missed this little feature, here it is.
By the way, I fixed the bower test since it didn't check the bower.json file. 😄
Thank for your great work.
Aurélien

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2a9c0c2 on noreiller:master into 13eb9cd on conventional-changelog:master.

@stevemao
Copy link
Member

stevemao commented Jun 3, 2017

Could you also add support for package-lock? Thanks :)

@bcoe
Copy link
Member

bcoe commented Jun 4, 2017

@stevemao the CLI team at npm tells me that you shouldn't publish the package-lock.json to the registry; and it looks like npm version (according to the docs) doesn't implement the version in package-lock.json

I say we land this feature, and I'll talk to my coworkers Monday to see if we would want to increment the version in package-lock.json.

@bcoe bcoe closed this Jun 4, 2017
@bcoe bcoe reopened this Jun 4, 2017
@bcoe bcoe merged commit 86af7fc into conventional-changelog:master Jun 4, 2017
@stevemao
Copy link
Member

stevemao commented Jun 5, 2017

you shouldn't publish the package-lock.json to the registry;

Did you mean to publish to the npm registry? The whole workflow we are dealing here doesn't include this step. We only checkin the changes to git. Also I believe npm publish doesn't allow you to publish this file and we shouldn't worry about it.

and it looks like npm version (according to the docs) doesn't implement the version in package-lock.json

I was looking at npm/npm@d654a8e

I say we land this feature, and I'll talk to my coworkers Monday to see if we would want to increment the version in package-lock.json.

Sounds good. I honestly don't know if version field should be in a lock file since it doesn't seem to add any extra information than the same field in package.json 😄

@bcoe
Copy link
Member

bcoe commented Jun 5, 2017

@stevemao I'll follow up tomorrow; and create an issue if the CLI team thinks we should bump the version in package-lock.json.

@zkat
Copy link

zkat commented Jun 5, 2017

You should. npm version does (and includes it in the commit). Right now, it's a minor annoyance 'cause the next install will update the version.

@bcoe
Copy link
Member

bcoe commented Jun 5, 2017

@noreiller, zkat literally wrote the book on this -- don't suppose I could convince you to send a follow up pull request.

@zkat
Copy link

zkat commented Jun 6, 2017

fwiw, y'all should treat npm-shrinkwrap.json and package-lock.json as interchangeable -- they're the same format, and anything this does to one, it should be able to do to the other.

@bcoe
Copy link
Member

bcoe commented Jun 6, 2017

@noreiller your changes are currently available in standard-version@next:

npm i standard-version@next, would love your help testing since it's a fairly major change.

also, would happily accept a follow on pull-request with support for package-lock.

@noreiller
Copy link
Contributor Author

Hi @bcoe, I just ran successful tests with standard-version@next.
Ok for another PR dedicated to package-lock.

@noreiller
Copy link
Contributor Author

PR #190 😄

@bcoe
Copy link
Member

bcoe commented Jun 6, 2017

@noreiller awesome 👍

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

Successfully merging this pull request may close these issues.

5 participants