-
Notifications
You must be signed in to change notification settings - Fork 520
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
Support for snapshot versioning and publish under custom tag #359
Conversation
🦋 Changeset is good to goLatest commit: c84899f We got this. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
=========================================
Coverage ? 80.32%
=========================================
Files ? 41
Lines ? 1103
Branches ? 258
=========================================
Hits ? 886
Misses ? 209
Partials ? 8
Continue to review full report at Codecov.
|
newVersion: | ||
snapshotConfig === undefined | ||
? incrementVersion(incompleteRelease, preInfo)! | ||
: `0.0.0-${snapshotConfig.tag}-${snapshotConfig.commitHash}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 0.0.0
should be used instead of the current version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the current version or the future version(as in the current version but with bumps applied) can cause unexpected behavior when combined with other pre-releases(e.g. you have a regular pre-release at 1.0.0-beta.0
and then you had a snapshot pre-release at 1.0.0-canary-git-hash
and a consumer is using the range ^1.0.0-beta
, most people would expect that range to resolve to 1.0.0-beta.0
but it'll actually resolve to 1.0.0-canary-hash
). Using 0.0.0
solves this problem because it won't conflict with other versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed explanation! I always forget that this is how canary versions are resolved. Might be good to add this as a comment in the code or/and an explanation in the docs about snapshot publish.
|
||
logger.log("All the files have been updated with snapshot release."); | ||
|
||
await publish(cwd, { otp: options.otp, tag: options.tag }, updatedConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to be able to use a custom publisher. I use pnpm publish -r
instead of changeset publish because it works better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a separate improvement maybe
packages/cli/src/index.ts
Outdated
(command === "snapshot" || command === "enter") && | ||
typeof tag !== "string" | ||
) { | ||
error(`A tag must be passed when using prerelese ${command}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the tag required when making a snapshot publish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a tag to know what npm dist-tag to publish to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, indeed. There is no way to publish without a tag because that way it would be "latest".
In that case, makes sense. Maybe some default tag could be used. Like the branch name, or "snapshot", or "dev". But that's just a good to have.
I don't understand what |
|
It seems to me as well that including "publish" somewhere in the command would make it easier to understand for newcomers, even at the cost of some added verbosity. |
Okay, let's make it |
f91d8d7
to
4e50b52
Compare
Thanks for the feedback everyone. :)
Please see the comments below now. #359 (comment) |
Currently, regular versioning and publishing scripts are separate - IMHO it would make sense to stick to that here as well without introducing My alternative proposal would be to add Q: why this has been bound to pre mode? Wouldn't it make sense to allow this outside of it? One could setup snapshot releases on each pr/commit without ever entering the pre mode - this use case seems useful. |
A: Snapshot release does not have to bound to pre release, it is just the significance that pre defines a pre-release. User does not have to be in pre mode snapshot release. Yes, I like the idea of Continuing on suggestion of keeping the version and publish separate I see this as:
Opinions? |
It makes sense to me, thanks. To summarize, let me try to apply this to some examples: Publishing to
Publishing to
Publishing canary releases
Publishing in "pre" mode
Did I get this right? |
Isn't this problem solved by splitting this into 2 commands? @zkochan could use versioning command to adjust files and just use a custom publish command, or have I missed something? |
Hey @Andarist, yes you are right. I misunderstood the second line. Sorry. I see using snapshot flag with version and using tag option with publish looks clean. As in #359 (comment) How can we decide on it? |
Probably a good idea to wait for @mitchellhamilton’s or @Noviny’s green light |
This sounds good to me. One little thing: changeset publish should throw if Unrelated to the above - instead of using the git hash, let's use a random hash so that it's not tied to git + you should be able to do multiple releases without the last commit hash changing |
Another good alternative to using hash would be to use date/time. I have noticed this in the typescript canary versions but they use only date. They have versions like this: 4.0.0-dev.20200512 |
4e50b52
to
4aa971d
Compare
9143780
to
cd6c547
Compare
FYI: React uses git SHA as well for canary/experimental releases
|
cd6c547
to
890e9e1
Compare
890e9e1
to
2c63232
Compare
Hey 👋, PR is ready for review. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so great @ajaymathur, thanks!!
Just some little comments
* but it'll actually resolve to 1.0.0-canary-hash. Using 0.0.0 solves this problem because it won't conflict with other versions. | ||
*/ | ||
// Creating cache of hash since this function is called for every release the value of second may change | ||
let uniqueHash: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cache this per assembleReleasePlan call rather than globally?
now.getHours(), | ||
now.getMinutes(), | ||
now.getSeconds() | ||
].join(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the UTC methods instead of the timezone-relative methods here? I don't think that the user's timezone should affect the release name. (e.g. two people in different time zones do releases(or you move from manual releases to doing releases on CI or etc.), you wouldn't want a newer release to have an older time)
options.snapshot === undefined && | ||
preState !== undefined && | ||
preState.mode === "pre" | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we throw here if snapshot is passed and we're in pre mode?(to give the user feedback before they're at the publish stage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why pre mode should clash with snapshot releases? seems like both could be used at the same time with success
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre mode changes the npm dist tag that's used and so does the --tag option
. Given you shouldn't commit the result of a snapshot release, it's easy to exit pre mode and then do the snapshot and you won't get any unexpected behavior. We can always change this behavior in the future if we want to in the direction of not erroring but going in the opposite direction is harder.
.changeset/dry-rivers-train.md
Outdated
"@changesets/cli": minor | ||
--- | ||
|
||
- Add support for snapshot flag to version command. Usage: `changeset version --snapshot [tag]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you describe the motivation and the use case for this?
The second thing (publish --tag
) is not tightly coupled to the --snapshot
so it could be moved to a separate changeset.
@@ -39,7 +39,8 @@ async function getCommitThatAddsChangeset(changesetId: string, cwd: string) { | |||
export default async function applyReleasePlan( | |||
releasePlan: ReleasePlan, | |||
packages: Packages, | |||
config: Config = defaultConfig | |||
config: Config = defaultConfig, | |||
snapshot?: string | boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why snapshot
is not part of config
or releasePlan
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re config: the config refers to the written config, this doesn't want to be part of that.
It could be part of the release plan, but would like to see it land and stabilise, as the release plan being relatively clean is nice.
…-release-plan's main function 2: [Version command] Add condition to throw error if snapshot version is made while changeset is in pre-state 3: [Publish command] Update the condision to capture preState.mode and updated the error message when tag is passed in pre state
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Thank you for your submission! Like many open source projects, we ask that you sign our CLA (Contributor License Agreement) before we can accept your contribution. Already signed the CLA? To re-check, try refreshing the page. |
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this @ajaymathur!!!
Hey @mitchellhamilton , thanks for a lot for accepting suggestion and merging the PR. Due to my travel arrangement this week I will not able to respond. Appreciate it. 🙂 |
Hey guys, when do you expect to publish this change? I can't wait to use it already. |
Resolves issue: #327
Changes:
1.
version
command now supports snapshot flag.Rule:
Usage:
2.
publish
command now supports tag flagRules:
Usage: