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

Add automatic provision of the --tag option to npm publish #56

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

Avaq
Copy link
Collaborator

@Avaq Avaq commented May 9, 2019

Closes #55

@Avaq Avaq requested a review from davidchambers May 9, 2019 08:07
@Avaq Avaq force-pushed the avaq/dist-tag branch 3 times, most recently from 63bb544 to 712b2cb Compare May 9, 2019 08:37
@davidchambers
Copy link
Owner

The "${options[publish-command]}" == 'npm publish' check doesn't sit well with me. Semantically, I believe we're trying to express the following:

if pre-release
  default-publish-command = npm publish --tag options[prerelease-label]
else
  default-publish-command = npm publish

if user-specified-publish-command
  publish-command = user-specified-publish-command
else
  publish-command = default-publish-command

The above could instead be expressed like so:

if user-specified-publish-command
  publish-command = user-specified-publish-command
else if pre-release
  publish-command = npm publish --tag options[prerelease-label]
else
  publish-command = npm publish

Rather than set the default value of publish-command conditionally, the code in this pull request updates the publish-command value if it is 'npm publish', regardless of whether the default value was used or whether the user specified 'npm publish' explicitly.

Although the subtle difference between the two approaches is unlikely to be significant in practice, I would find the code easier to understand if it only manipulated the default publish-command value.

@Avaq
Copy link
Collaborator Author

Avaq commented May 9, 2019

I knew you'd say that. I tried going for the suggested approach first, but found that in order to run this logic:

if user-specified-publish-command
  publish-command = user-specified-publish-command
else if pre-release
  publish-command = npm publish --tag options[prerelease-label]
else
  publish-command = npm publish

Two separate variables are needed (user-specified-publish-command and publish-command), which needed various changes to the code.

I figured by using the right wording in the documentation, I could get away with the current approach.

@davidchambers
Copy link
Owner

Could we leave options[publish-command] unset initially, and set it to the desired default value later if it remains unset?

@Avaq
Copy link
Collaborator Author

Avaq commented May 9, 2019

Sure :)
I thought you'd disapprove because of the mixed type, in that case.

@Avaq
Copy link
Collaborator Author

Avaq commented May 11, 2019

I just released a new beta version of Fluture using this version of xyz. I'm happy to report that it works as intended. :)

$ npm dist-tags ls fluture
beta: 12.0.0-beta.1
latest: 11.0.1

@Avaq
Copy link
Collaborator Author

Avaq commented Jun 11, 2019

Shall we merge and release this?

@davidchambers
Copy link
Owner

Shall we merge and release this?

Yes. Thank you for reminding me. v3.1.0 or v4.0.0?

@Avaq
Copy link
Collaborator Author

Avaq commented Jun 12, 2019

v3.1.0 or v4.0.0?

Technically I think this is a breaking change, because the behaviour changed. I would be surprised if anyone relies on prerelease packages being published with the latest tag, but I still think it's good to do a major bump, if only to give people a reason to look at what's changed.

xyz Outdated Show resolved Hide resolved
@Avaq
Copy link
Collaborator Author

Avaq commented Sep 9, 2019

@davidchambers Would you like me to merge, and perhaps release, this?

@davidchambers
Copy link
Owner

Absolutely. In the future please let me know when you git push --force a change as I rely on GitHub notifications to know when action is required from me. I'm sorry to have left you hanging.

@davidchambers davidchambers merged commit 0017b71 into master Sep 9, 2019
@davidchambers davidchambers deleted the avaq/dist-tag branch September 9, 2019 21:35
@Avaq
Copy link
Collaborator Author

Avaq commented Nov 1, 2019

Hi @davidchambers. Any plans to release this to npm? I've been using direct install from GitHub all this time.

@davidchambers
Copy link
Owner

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.

Use --prerelease-label to supply the --tag option to npm publish
2 participants