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

Prepare project for a new life as a Sinon.JS project #7

Merged
merged 13 commits into from Mar 2, 2018

Conversation

mroderick
Copy link
Member

This PR fixes most of the tasks in #6

@mroderick mroderick requested a review from mantoni March 2, 2018 12:16
.npmrc Outdated
@@ -0,0 +1 @@
save-exact=true
Copy link
Member

Choose a reason for hiding this comment

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

This is what package-lock.json is for. If we have fixed versions for dependencies, not even patches would be updated on a fresh install of this module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some projects published to npm do not follow semver. I got bitten by this recently, and the reply was basically that they didn't care about it

Copy link
Member

Choose a reason for hiding this comment

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

Was it one of the dependencies of this project?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a dependency of one of the @sinonjs projects, trying to find it now

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that only the one dependency, where the maintainers use npm without following the official npm semantics, should be a fixed version. Giving up on semver entirely just because someone else doesn't like it means we're actively helping to kill it.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know. I also depend on them in some projects. Do we have an alternative module?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that any of the sinonjs projects need to ship minified versions at all. I'd be happy to remove it from sinon-test, and then update all repositories to use default npm behaviour (which I am not a huge fan of, but it's the well beaten path)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update this PR to remove that commit, and set dependencies back to normal

Copy link
Member Author

Choose a reason for hiding this comment

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

done

# https://github.com/nodejs/LTS
- "4" # ends April 2018
- "6" # ends April 2019
- "8" # ends December 2019
Copy link
Member

Choose a reason for hiding this comment

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

This is very helpful! 😎

Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢

@mroderick mroderick merged commit 0af5fdb into sinonjs:master Mar 2, 2018
@mroderick mroderick deleted the new-hotness branch March 2, 2018 12:58
@mroderick
Copy link
Member Author

Thank you :)

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.

None yet

2 participants