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

Update devDependencies #3277

Closed
wants to merge 1 commit into from

Conversation

yoshinorin
Copy link
Member

Some package are out of date.
This PR is update devDependencies and upgraded husky's script, because it has breaking change in v1.0.0

  • Add test cases for the changes.
  • Passed the CI test.

* upgraded husky's script, because it has breaking change in v1.0.0
@coveralls
Copy link

coveralls commented Oct 1, 2018

Coverage Status

Coverage decreased (-0.3%) to 97.012% when pulling 9624dbb on YoshinoriN:update-devDependencies into 0b26940 on hexojs:master.

"lint-staged": "^7.3.0",
"mocha": "^5.2.0",
"rewire": "^4.0.1",
"sinon": "^6.3.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO think the version difference is very large even for the sinon library alone, and it is not appropriate for the update target at once.

Copy link
Member Author

@yoshinorin yoshinorin Oct 2, 2018

Choose a reason for hiding this comment

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

Thanks!

At first about sinon.As you know This PR update sinon 4.x to 6.x.
sinon breaking changes in 5.x and 6.x is only removed spt.reset(). But this repositories doesn't using spy.reset().

Please see sinon migrate guide.

Second about rewire. rewire 4.0.0 has two breaking changes.
One is drop nodejs v4. This hexo repository already set node-engines higher 6.9.0, so we don't worry this breaking change.
Another one is potentially breaking, sorry I can not be say absolutely no problem, but unit test are passed. I think may be no problem.

So, I think we can bump up these dependencies to higher major version. But, if hexojs's committer require more carefully, I will change them major version up to minor version up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @yoshinorin for your quick reply!
I think there is no problem with the change itself.
Since the version difference is simply very large, I was worried about whether it is appropriate for PR unit.
It was a pleasure for you to leave the content of breaking change as a comment!

Including the reply to the review, I thought that I could merge. However, I do not know whether it is correct as a guideline for the hexojs project.

@yoshinorin
Copy link
Member Author

yoshinorin commented Oct 10, 2018

OK. I wait other hexo core member's review.
I will try to divide this PR into some PRs if this PR will not accepte or take a long time.

@NoahDragon
Copy link
Member

I agree with @segayuu, but since the upgrades packages are all in dev. IMHO, I feel it is safe to merge as it passes the CI test.

@yoshinorin
Copy link
Member Author

@segayuu @NoahDragon
Thanks.
OK, I will close this PR and I divide this PR into some PRs.

@yoshinorin yoshinorin closed this Nov 3, 2018
@yoshinorin yoshinorin deleted the update-devDependencies branch November 3, 2018 12:36
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.

4 participants