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

New config flag: snapshot.prereleaseTemplate, and make snapshot feature stable #858

Merged
merged 24 commits into from Jul 22, 2022

Conversation

dotansimha
Copy link
Contributor

@dotansimha dotansimha commented Jun 29, 2022

Closes #855 and closes #830

Stable snapshot feature

This PR changes the stability state of the snapshot feature. You can now use and declare the configuration under the root of config.json.

Before:

{
  "___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH": {
    "useCalculatedVersionForSnapshots": true
  }
}

After:

{
    "snapshot": {
        "useCalculatedVersion": true
    }
}

New snapshotPreidTemplate

Added a new config option: snapshotPreidTemplate for customizing the way snapshot release numbers are being composed.

Available placeholders

You can use the following placeholders for customizing the snapshot release version:

  • {tag} - name of the tag, as specified in --snapshot something
  • {commit} - the Git commit ID
  • {timestamp} - Unix timestamp of the time of the release
  • {datetime} - date and time of the release (14 characters, for example: 20211213000730)

Note: if you are using --snapshot with empty tag name, you cannot use {tag} as placeholder - this will result in error.

Integration with useCalculatedVersion

You can still use and pass snapshot.useCalculatedVersion: boolean if you wish to have the snapshot releases to based on the planned release of changesets, instead of 0.0.0.

Legacy mode

If you are not specifying snapshot.prereleaseTemplate, the defualt behaviour will fallback to use the following template: {tag}-{datetime}, and in cases where tag is empty (--snapshot with no tag name), it will use {datetime} only.

@changeset-bot
Copy link

changeset-bot bot commented Jun 29, 2022

🦋 Changeset detected

Latest commit: d046aaa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@changesets/cli Minor
@changesets/config Minor
@changesets/git Minor
@changesets/assemble-release-plan Minor
@changesets/types Minor
@changesets/apply-release-plan Patch
@changesets/get-release-plan Patch
@changesets/read Patch
@changesets/release-utils Patch
@changesets/changelog-git Patch
@changesets/changelog-github Patch
@changesets/get-dependents-graph Patch
get-workspaces Patch
@changesets/parse Patch
@changesets/pre Patch
@changesets/write Patch

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

@dotansimha dotansimha changed the title wip Drop useCalculatedVersionForSnapshots and use snapshotVersionTemplate instead Jun 29, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 29, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d046aaa:

Sandbox Source
Vanilla Configuration

@dotansimha dotansimha marked this pull request as ready for review Jun 29, 2022
@Andarist
Copy link
Member

Andarist commented Jun 29, 2022

re snapshotVersionTemplate - I don't think that the whole version field should be configurable like this. It would allow people to generate invalid versions etc. IMHO only the suffix should be configurable - this way we wouldn't have to validate what got generated etc

It should still be possible to switch to using a planned version using a boolean config or something.

This PR also drops the expetimental useCalculatedVersionForSnapshots and use snapshotVersionTemplate instead.

I don't think we want to drop it just yet - it would be better to raise a warning when it's used. Even though it was an experimental option it is definitely used by some projects.

{tag} - the name of the tag. Please note that in the case of using --snapshot (without tag name), this will be empty. Otherwise, the value will be replaced with -{tag} to avoid -- in the case of an empty tag name.

I didn't look into the code itself but it seems like those are some funky rules here. Wouldn't it be a safe assumption that people are always using --snapshot tag or --snapshot but they are never "mixing" it? Could we just throw when ${tag} is encountered in a pattern but it has not been provided? Could we throw when ${tag} is not encountered but a custom tag has been provided?


I also think that we feel somewhat confident about this feature - it's only a matter of bikeshedding the details and stuff so we should introduce this as a stable option. We don't have to introduce each new thing within the experimental namespace, that is reserved for things that seem useful but for which not everything is clear, has been hashed out, etc

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jun 29, 2022

We use useCalculatedVersionForSnapshots: true and TBH I think that should be the default, and definitely not removed; I very much like using snapshots for prereleases (without committing changed package.jsons and changelogs).

@Andarist
Copy link
Member

Andarist commented Jun 30, 2022

We use useCalculatedVersionForSnapshots: true and TBH I think that should be the default, and definitely not removed; I very much like using snapshots for prereleases (without committing changed package.jsons and changelogs).

Note that this PR doesn't remove the ability to do this - it just accomplishes it in a different way. I agree though that we shouldn't immediately remove the old way of doing this, to allow people to migrate things at their own pace.

@dotansimha
Copy link
Contributor Author

dotansimha commented Jun 30, 2022

re snapshotVersionTemplate - I don't think that the whole version field should be configurable like this. It would allow people to generate invalid versions etc. IMHO only the suffix should be configurable - this way we wouldn't have to validate what got generated etc

We do have checks for valid semver at a later step if I remember correctly, but I see your point. If only suffix is configurable, I guess it's only a matter of having useCalculatedVersionForSnapshots: boolean and snapshotsSuffix: 'commitId' | 'timestamp'? With the tag being specified anyway through the CLI.

Note that this PR doesn't remove the ability to do this - it just accomplishes it in a different way. I agree though that we shouldn't immediately remove the old way of doing this, to allow people to migrate things at their own pace.

With this note added, I guess the solution I suggested above might be ideal? (avoids the semver issues, supports for customizing the snapshot version, and doesn't deprecate/change existing config flags?)

If that's the direction you prefer to go, please let me know and I'll update this PR accordingly :)

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jul 1, 2022

Note that this PR doesn't remove the ability to do this - it just accomplishes it in a different way. I agree though that we shouldn't immediately remove the old way of doing this, to allow people to migrate things at their own pace.

With this note added, I guess the solution I suggested above might be ideal? (avoids the semver issues, supports for customizing the snapshot version, and doesn't deprecate/change existing config flags?)

If that's the direction you prefer to go, please let me know and I'll update this PR accordingly :)

I was trying to do something similar in this PR #830 and i realized that - at least for my use case - I would not only like to use simple "templates" with tokens (like in this PR), but also have a new CLI flag allowing to specify snapshot template for that specific run of changset version.

Anyway, I'm happy to help.

@Andarist
Copy link
Member

Andarist commented Jul 1, 2022

If only suffix is configurable, I guess it's only a matter of having useCalculatedVersionForSnapshots: boolean and snapshotsSuffix: 'commitId' | 'timestamp'? With the tag being specified anyway through the CLI.

People have different expectations as to where the tag should be put, some could also want to include both commitId and timestamp in the template. So maybe a template is still desirable here? I'm not entirely sure about it though - WDYT? cc @mitchellhamilton

You can also actually attach "random" build metadata (it can be just anything) to your package.json#version after a plus sign so by using a template we would allow people to use this. It's not a very popular thing though - I only learned about this possibility a couple of months back. I've seen it being used in the wild though:
https://unpkg.com/browse/@react-aria/toggle@3.0.0-nightly.1651/package.json#L3

would not only like to use simple "templates" with tokens (like in this PR), but also have a new CLI flag allowing to specify snapshot template for that specific run of changset version.

I'm curious - what's your use case for configuring a template for a specific changeset version run?

@emmatown
Copy link
Member

emmatown commented Jul 1, 2022

IMHO only the suffix should be configurable - this way we wouldn't have to validate what got generated etc

I don't feel that strongly about having the template be the entire version but we would still have to validate the template anyway since you could put in characters that aren't valid as a suffix. We could trivially validate it once by replacing the placeholders with some static values and checking if it's a valid version so what's the harm in making the template be for the whole version?

People have different expectations as to where the tag should be put, some could also want to include both commitId and timestamp in the template. So maybe a template is still desirable here? I'm not entirely sure about it though - WDYT?

Yeah, this is why I suggested a template. Having the order of things, the separator and what things should appear in a bunch of separate options just seems confusing, having a template makes it easy to see what it'll actually be.

@Andarist
Copy link
Member

Andarist commented Jul 1, 2022

We could trivially validate it once by replacing the placeholders with some static values and checking if it's a valid version so what's the harm in making the template be for the whole version?

If the whole version is the template then this is a valid template:

'1.2.3'

I don't think we should allow people to tamper with the version itself - it should either be the computed one (our responsibility) or 0.0.0 (a static value). I don't feel suuuper strongly about it but it seems to me that allowing arbitrary versions at the leading position is a footgun.

@emmatown
Copy link
Member

emmatown commented Jul 1, 2022

If the template is only the suffix, you could make it an empty string and get versions like 0.0.0- or make it some static thing and you'd have a problem if you tried to publish twice, so it's the same, no?

@Andarist
Copy link
Member

Andarist commented Jul 2, 2022

Hm, sure - but with a static suffix, we won't attempt to publish the same version multiple times. It won't do anything useful, it will just skip over the publish step. If we give complete control over the version to users then it will be "easy" to publish 1.0.0 (no prerelease suffix at all!) when your latest version is already 2.3.4 (or whatever).

Perhaps this is just a territory of "please don't do stupid things" but it seems to me that if we can avoid that then it would be better. The drawback is though that we'd complicate our options API a little bit.

I'll leave the decision about this to you - those are just my concerns, I'm not giving a veto to the single template idea.

@emmatown
Copy link
Member

emmatown commented Jul 2, 2022

If we give complete control over the version to users then it will be "easy" to publish 1.0.0 (no prerelease suffix at all!) when your latest version is already 2.3.4 (or whatever).

Yeah, that's fair, happy to have just a template for the suffix

@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 3, 2022

Yeah, that's fair, happy to have just a template for the suffix

Cool, so we'll go with:

useCalculatedVersionForSnapshots: boolean (default: false)
snapshotVersionSuffix: 'timestamp' | 'commit' (default: 'timestamp')

So this way, the actual version part is controlled internally (can be customized using useCalculatedVersionForSnapshots), the tag is controller using the CLI option, and the suffix can be either a timestamp of a commit ID.

Makes sense? @Andarist @mitchellhamilton Please let me know so I'll change this PR accordingly. thanks!

@Andarist
Copy link
Member

Andarist commented Jul 3, 2022

I think we've agreed on using the template for the suffix itself and I would go with that here, otherwise, we'll end up with people wanting to customize this further.

@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 3, 2022

I think we've agreed on using the template for the suffix itself and I would go with that here, otherwise, we'll end up with people wanting to customize this further.

Got it, thanks! I'm working on changing this PR :) Will update soon

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jul 3, 2022

So this way, the actual version part is controlled internally (can be customized using useCalculatedVersionForSnapshots), the tag is controller using the CLI option, and the suffix can be either a timestamp of a commit ID.

Does that mean I won't be able to use both a timestamp and a tag (from the CLI option) at the same time? That would be backwards incompatible with how it currently works.

@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 3, 2022

So this way, the actual version part is controlled internally (can be customized using useCalculatedVersionForSnapshots), the tag is controller using the CLI option, and the suffix can be either a timestamp of a commit ID.

Does that mean I won't be able to use both a timestamp and a tag (from the CLI option) at the same time? That would be backwards incompatible with how it currently works.

Nope, it means that you can still use --snapshot for empty tag, or --snapshot alpha for a named tag.
The suffix will be either timestamp or commit ID.

Here's an example of the config set and their result, does it makes sense? @jakubmazanec

cli: --snapshot
output: 0.0.0-1656838548676
cli: --snapshot alpha
output: 0.0.0-alpha.1656838548676
cli: --snapshot
useCalculatedVersionForSnapshots: true
output: 1.2.3-1656838548676
cli: --snapshot canary
useCalculatedVersionForSnapshots: true
output: 1.2.3-canary.1656838548676
cli: --snapshot canary
useCalculatedVersionForSnapshots: true
snapshotVersionSuffix: commit
output: 1.2.3-canary.abcdef
cli: --snapshot
snapshotVersionSuffix: commit
output: 0.0.0-abcdef

This way, we get the following:

  • Changesets internals is in charge of building the actual version.
  • User is in charge of providing the optional tag name to be used.
  • User can choose to use 0.0.0 for snapshot, or planned version.
  • User can choose which suffix to use (timestamp or commit ID)
  • Snapshot releases will always have a suffix, as required by NPM.

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jul 3, 2022

@dotansimha Yes, thank you for the explanation. But I would welcome if I could 1) use both timestamp and commit id at the same time, and 2) sometimes reverse location of timestamp/commit and tag. Also, you use . as preid separator, while currently it's -, so that needs to be configurable too. I don't think all that can be configured without some simple templating, as mentioned earlier in this discussion (otherwise we could just use #830 right away).

Anyway, I was thinking how to then solve the situations like 1) when the tag from CLI option is empty, resulting in invalid preid (e.g. 0.0.0-.timestamp), or 2) different snapshot suffix for different "types" of prereleases (e.g. nightly release with only commit id vs feature branch release with only timestamp), without including some sort of if/then/else logic to the template. I'm still coming back to this solution: new CLI option for overriding the default snapshotPreidTemplate option from config, e.g.:

snapshotPreidTemplate: {{tag}}.{{timestamp}} 
cli: --snapshot foobar
result: 0.0.0-foobar.timestamp

snapshotPreidTemplate: {{tag}}.{{timestamp}} 
cli: --snapshot 
result: ERROR

snapshotPreidTemplate: {{tag}}.{{timestamp}} 
cli: --snapshot --snapshotPreidTemplate {{timestamp}} 
result: 0.0.0-timestamp

@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 3, 2022

Thanks for the feedback @jakubmazanec !

  1. use both timestamp and commit id at the same time

This might be valid use-case (I never tried to mix the both, but I can understand where it's coming from) - @Andarist @mitchellhamilton what do you think?

  1. sometimes reverse location of timestamp/commit and tag.

Yeah, there is no clear spec in semver about that - you can put whatever you want after the -. I usually do {tag}.{commit}.

Regarding next steps - I'm fine with both solutions TBH - regarding empty tag - I think we should either choose to prevent it (does it make sense to publish something like 1.2.3-test as tag=alpha on NPM? It's allowed but kinda weird), or keep it as-is (where the code treats it as part of the suffix). Today the CLI can handle empty tag because there is a default timestamp added, so you get a valid suffix always (even when it's empty).

@Andarist @mitchellhamilton @jakubmazanec I guess now it's a matter of a decision? Are we going with a new config param for the suffix (timestamp/commit), or with a template for the entire suffix (tag + timestamp/commit/anything)?

@Andarist
Copy link
Member

Andarist commented Jul 3, 2022

or with a template for the entire suffix (tag + timestamp/commit/anything)?

This

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jul 3, 2022

I vote, of course, for template for the entire suffix (with CLI option to override it). I think it will provide the greatest number of possible prerelease version formats and thus satisfying most users. We can then check if the preid is nonempty (so no 0.0.0- version) and then use Semver regexp to make sure there are no illegal characters. The rest is users' responsibility - if they want to publish version like 1.2.3-test, it's their choice.

  1. use both timestamp and commit id at the same time

This might be valid use-case (I never tried to mix the both, but I can understand where it's coming from)

Timestamp is useful because if placed at the beginning of the preid, it solves the prerelease sorting issue. And commit id identifies the commit, that's obviously useful for testing, debugging and stuff.

@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 4, 2022

@jakubmazanec @Andarist thanks, I'm on it :)

What about the default value for snapshotPreidTemplate? Should it be {tag}.{timestamp} ? what would happen in a case of --snapshot? (with no tag name) Should this maybe be {tag.}{timestamp} as default? 🤔
Otherwise, we need another way to represent the possibility of having "" as tag, and then ignore the . maybe?
Should we just leave it as is? So it's a user's fault if they use {tag}-{something} and pass --snapshot?

@jakubmazanec you mentioned this as ERROR, but that's a valid state at the moment if I understand correctly. Should we change that?

@jakubmazanec
Copy link
Contributor

jakubmazanec commented Jul 4, 2022

Good point, we can't introduce such breaking change 🤔 I think the default for snapshotPreidTemplate should be undefined, and in that case the old (i.e. current) behavior for generating snapshot preid would be used (i.e. 0.0.0-tag-timestamp, or 0.0.0-timestamp). If you define snapshotPreidTemplate (be it via config or CLI option), you opt-in to the new behavior and then we can throw error if the result of using the template would be invalid semver.

@Andarist
Copy link
Member

Andarist commented Jul 4, 2022

I agree with @jakubmazanec. It seems the most straightforward to do this. We can't have a single default without some special logic around it anyway, so best to treat the lack of a template in a special way.

I will also quote what I've written a couple of days ago here as I feel it's still relevant:

Wouldn't it be a safe assumption that people are always using --snapshot tag or --snapshot but they are never "mixing" it? Could we just throw when {tag} is encountered in a pattern but it has not been provided? Could we throw when {tag} is not encountered but a custom tag has been provided?

@dotansimha dotansimha changed the title New config flag: snapshotPrereleaseTemplate to allow customizing the snapshot release version New config flag: snapshot.prereleaseTemplate to allow customizing the snapshot release version Jul 17, 2022
.changeset/olive-ducks-camp.md Outdated Show resolved Hide resolved
@Andarist
Copy link
Member

Andarist commented Jul 17, 2022

@dotansimha sorry, I was on a company retreat and didn't have much time to look into GitHub. I've only left a single comment about the breaking change stuff. Otherwise this LGTM.

@dotansimha dotansimha changed the title New config flag: snapshot.prereleaseTemplate to allow customizing the snapshot release version New config flag: snapshot.prereleaseTemplate, and make snapshot feature stable Jul 18, 2022
@dotansimha
Copy link
Contributor Author

dotansimha commented Jul 18, 2022

@dotansimha sorry, I was on a company retreat and didn't have much time to look into GitHub. I've only left a single comment about the breaking change stuff. Otherwise this LGTM.

No problem! I pushed a fix to avoid the breaking changes and make the snapshot config stable :) I think we should good to go now 🚀

@dotansimha dotansimha requested a review from Andarist Jul 18, 2022
@Andarist
Copy link
Member

Andarist commented Jul 18, 2022

Thank you @dotansimha for bearing with me :) The PR LGTM - atm I'm in the middle of things and I would like to merge this with a clear head, gonna take another look at this soon, just double-checking everything etc. If I notice anything to tweak then I will do it myself to avoid the extra cycle of 🏓 I expect this to get merged in within the next 3 days or so.

.changeset/strong-geckos-divide.md Outdated Show resolved Hide resolved
.changeset/olive-ducks-camp.md Outdated Show resolved Hide resolved
docs/experimental-options.md Outdated Show resolved Hide resolved
packages/cli/src/index.ts Outdated Show resolved Hide resolved
@Andarist Andarist enabled auto-merge (squash) Jul 22, 2022
@Andarist Andarist merged commit dd9b76f into changesets:main Jul 22, 2022
4 checks passed
@github-actions github-actions bot mentioned this pull request Jul 22, 2022
@dotansimha
Copy link
Contributor Author

dotansimha commented Aug 2, 2022

Amazing, thank you for taking this one to the finish line @Andarist !

@dotansimha dotansimha deleted the snapshot-version-template branch Aug 2, 2022
@Andarist
Copy link
Member

Andarist commented Aug 2, 2022

Thank you for the amazing contribution and for bearing with me 😉

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.

Support for canary releases based on changesets?
5 participants