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

Removes dependency to "build" #1563

Closed
wants to merge 1 commit into from
Closed

Removes dependency to "build" #1563

wants to merge 1 commit into from

Conversation

vsa
Copy link
Contributor

@vsa vsa commented Sep 18, 2017

Purpose (TL;DR)

I noticed that npm install started complaining about some outdated sub-dependencies after switching to Sinon version 3.3.

Background (Problem in detail)

build was added as a dependency in 85f30b5. I think that was a mistake (maybe a typo on the command line)?

How to verify

  1. Check out this branch
  2. npm install
  3. Look for the following deprecation message (it should no longer appear):

npm WARN deprecated [email protected]: wrench.js is deprecated! You should check out fs-extra (https://github.com/jprichardson/node-fs-extra) for any operations you were using wrench for. Thanks for all the usage over the years.

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

This was added in version 3.3 as part of commit [https://github.com/sinonjs/sinon/commit/85f30b5e2b73676acb0100e2d39727abf83b995c](85f30b5e). I think that was a mistake (maybe a typo on the command line)? I noticed because `npm install` started complaining about some outdated sub-dependencies.
@mroderick
Copy link
Member

package-lock.json was also introduced in that commit. Was that intentional @fatso83?

@fatso83
Copy link
Contributor

fatso83 commented Sep 18, 2017

Didn't notice the added dependency as I already touched the project file ... Argh. Was a coincidental artifact of trying to build the project.

@mroderick, yeah that was intentional, since NPM has been bugging me about this ever since Node 7. Seemed like a good idea, but I should perhaps run it passed you in a PR ... Sorry about that - since it's whole point is to keep things as is, I assumed it was an ok addition..

@mroderick
Copy link
Member

@mroderick, yeah that was intentional, since NPM has been bugging me about this ever since Node 7. Seemed like a good idea, but I should perhaps run it passed you in a PR ... Sorry about that - since it's whole point is to keep things as is, I assumed it was an ok addition..

I am in favour of locking down dependencies as hard as we can, so I think we should just leave package-lock.json there. However, we will have to make sure that it gets updated whenever we update dependencies part of package.json.

As for the build dependency, I think we can just update this PR with changes to package-lock.json and then merge it

@fatso83
Copy link
Contributor

fatso83 commented Sep 18, 2017

OK, so basically @vsa should redo npm uninstall build using Node 8 / NPM 5.3

@fatso83 fatso83 mentioned this pull request Sep 18, 2017
@fatso83
Copy link
Contributor

fatso83 commented Sep 18, 2017

Fixed the omissions by recreating as #1564. Closing this.

@fatso83 fatso83 closed this Sep 18, 2017
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