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

Do not test new version with pkg.version #3103

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Do not test new version with pkg.version #3103

merged 1 commit into from
Apr 21, 2017

Conversation

binarykitchen
Copy link
Contributor

See issue 3011 for details. Leave it to npm registry to validate the version number.

Motivation is to make it work with the way how I deploy my videomail-client library at
https://github.com/binarykitchen/videomail-client/blob/develop/bin/release.sh

There I am already bumping to the new version in package.json before publishing it with yarn to npm.

Test plan

  1. Manually increment version in a random package.json
  2. Run yarn publish --new-version with that new version
  3. Yarn won't complain anymore that the new version is the same.
  4. It will be up to the npm registry whether to accept that version or not.

@voxsim
Copy link
Contributor

voxsim commented Apr 11, 2017

hey @binarykitchen thanks for your pr, could you please write two tests for both cases instead of leaving a comment?

@binarykitchen
Copy link
Contributor Author

not sure how this can be tested when it's up to the npm registry to validate version numbers ...

@binarykitchen
Copy link
Contributor Author

@voxsim ok, have code to cover the first test case

but there are no tests at all for the yarn publish command, so cannot add the second test case. maybe make an empty file for now with todo comments and raise this as a separate issue?

@voxsim
Copy link
Contributor

voxsim commented Apr 14, 2017

@binarykitchen maybe for now it is sufficient. Let @arcanis and @bestander decide if we need to do other stuff ;)

// because of two reasons (see issue #3011):
// - npm registry will complain anway when it already exist (leave it to them to validate that)
// - some open source libraries/projects increment version just before deployment where
// the new version already has been bumped in package.json
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this comment is needed.

However, shouldn't we just return early when the package version is the same as the version currently registered? Instead of running lifecycle hooks etc. Not doing this could cause issues if, for example, a version lifecycle script was registering a mercurial tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true that @arcanis

... just have updated PR. now it's returning early and comment is much shorter

@arcanis
Copy link
Member

arcanis commented Apr 18, 2017

👍 Would you mind also adding a --no-version option on the publish command to completely skip the version step?

@binarykitchen
Copy link
Contributor Author

@arcanis i think that would raise scope and should got in a separate issue. my commit here only touches the version command. or am i missing something here?

@binarykitchen
Copy link
Contributor Author

@arcanis ping?

@arcanis
Copy link
Member

arcanis commented Apr 20, 2017

I'm ok with merging this! Just a nit: can you remove the comments? Such a small change is relatively self-explanatory, so no need to write in english what's already written in js :)

See issue 3011 for details. Leave it to
npm registry to validate the version number.
@binarykitchen
Copy link
Contributor Author

@arcanis ok, all comments are removed - good now?

@bestander bestander merged commit e7e2aa7 into yarnpkg:master Apr 21, 2017
@bestander
Copy link
Member

Great job, everyone!

@binarykitchen
Copy link
Contributor Author

thanks - is there an eta when and in which version this will come out @bestander?

@bestander
Copy link
Member

0.24 next week

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