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

Implement changeset tag and getAllTags. Resolves #612. #634

Merged
merged 12 commits into from Nov 7, 2021

Conversation

joeldenning
Copy link
Contributor

See #612. This is my first pull request to this repository - let me know if I missed any steps for contributing.

@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2021

🦋 Changeset detected

Latest commit: 608d566

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

This PR includes changesets to release 2 packages
Name Type
@changesets/cli Minor
@changesets/git Minor

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

@atlassian-cla-bot
Copy link

atlassian-cla-bot bot commented Sep 8, 2021

Hooray! All contributors have signed the CLA.

packages/git/src/index.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/tag/index.ts Show resolved Hide resolved
packages/git/src/index.ts Outdated Show resolved Hide resolved
@@ -26,6 +26,16 @@ async function commit(message: string, cwd: string) {
return gitCmd.code === 0;
}

async function getAllTags(cwd: string): Promise<string[]> {
const gitCmd = await spawn("git", ["tag"], { cwd });
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly worried that tags might not always be available on CI when working with shallow gi clones and similar setups. I'm no git expert so this would require some research on the topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read through https://git-scm.com/docs/git-clone and tested locally with git clone --depth=1. I saw that the tags are not included in a shallow clone with --depth=1. Also, they are not included when the --no-tags flag is passed to git clone.

Different CI/CD tools run git clone with a wide variety of flags, so there may be some tools that pass the --depth or --no-tags flag. I looked at github's actions/checkout and it allows you to configure the --depth flag but I see no mention of the --no-tags flag. This documentation indicates that it's quite easy to get all the git tags and branches, if needed, by setting the depth to 0. I didn't look at travis, circle, or any other CI/CD tools.

Copy link
Member

Choose a reason for hiding this comment

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

I'll give this a thought - I guess the answer could be: "make the user responsible for making tags available" 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's reasonable that the changeset tag command would require that you've cloned the tags along with the repo. It's in the name :)


The tag command creates git tags for the current version of all packages. The tags created are equivalent to those created by [`changeset publish`](#publish), but the `tag` command does not publish anything to npm.

This is helpful in situations where a different tool, such as `pnpm publish -r`, is used to publish packages instead of changeset. For situations where `changeset publish` is executed, running `changeset tag` is unneeded. See [issue 612](https://github.com/atlassian/changesets/issues/612) for the original inspiration behind this command.
Copy link
Member

@Andarist Andarist Oct 10, 2021

Choose a reason for hiding this comment

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

I know that currently there are some issues with pnpm, but I'm thinking about improving the story around that and implementing a better publishing strategy with changesets that would account for more situations. If that would be fixed in Changesets itself - would there still be a reason to choose pnpm publish -r over changeset publish?

This doesn't necessarily mean that changeset tag would be redundant. I guess one can always have some reasons to implement the publish part of the process on their own and changeset tag makes things more flexible for those use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that currently there are some issues with pnpm, but I'm thinking about improving the story around that and implementing a better publishing strategy with changesets that would account for more situations. If that would be fixed in Changesets itself - would there still be a reason to choose pnpm publish -r over changeset publish?

pnpm supports workspace:* as a valid semver range value for a package.json dependency, which allows you to cross reference packages and ensure that when you publish, each package always uses the exact version of all monorepo dependencies. During publish, it replaces the workspace:* with a pinned version of the dependency. You can see that in this example where generator-single-spa is pinned to 4.1.2, even though the source code just says workspace:*. Additionally, pnpm publish -r runs all prepublish and prepublishOnly scripts in each package in the monorepo. Since I don't use changeset publish right now, I don't know how much of an overlap there is. @zkochan could comment further on any other features of pnpm publish -r that i'm unaware of

Copy link
Member

Choose a reason for hiding this comment

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

Both of the things mentioned could be supported by the changeset publish. The latter already is, since we literally call npm publish in the given directory. My plan for the former is to invoke the appropriate pack command and publish that (so replacing would happen within pnpm), this would - most likely - require us to also implement running some lifecycle scripts within changeset (instead of relying on them being executed within npm publish) but as far as I remember this (running the scripts) is exactly what pnpm does under the hood. So it looks like a valid strategy for us too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me, for me it's just about getting the right behavior from whichever tools can provide it.

Copy link
Contributor

Choose a reason for hiding this comment

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

npm publish will not work correctly in a pnpm workspace.

Copy link
Member

Choose a reason for hiding this comment

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

@zkochan using smth like pnpm pack && npm publish lib.tar.gz would work though, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

right

Comment on lines 6 to 10
Add `changeset tag` and `getAllTags`

The `changeset tag` command creates git tags for each package.

The `getAllTags` function within @changesets/git retrieves all git tags for a repository.
Copy link
Member

Choose a reason for hiding this comment

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

I think I would split this changeset file into 2. The getAllTags part is not relevant for people reading about the new feature in@changesets/cli.

Don't be afraid of describing a single PR from different angles :) Each touched package might often need a different summary of changes (the change should be - IMHO - described primarily from the perspective of that package).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏿 will update

@joeldenning
Copy link
Contributor Author

Thanks for the thorough review - I'm not sure when I'll have time to address the changes, but hopefully in the next week.

@Andarist
Copy link
Member

@joeldenning no worries. Sorry for my late reply anyway - i was lately busy and didnt have time to work on Changesets much but i think i got some fresh energy for that (still not a lot of time though 😜). Hopefully, i should be more responsive in the coming weeks.

If you dont end up addressing the issues I will eventually pick this up and bring this through a finish line.

I would appreciate an answer on the question asked here though: #634 (comment) . Im rethinking how to reimplement publishing and this would help me to consider your use case.

@joeldenning
Copy link
Contributor Author

I pushed a couple new commits addressing the feedback, and responded to your question above.

@joeldenning
Copy link
Contributor Author

I've responded to all comments and think I've addressed all of the review feedback - let me know if there's anything I looked over or other things to change.

@Andarist
Copy link
Member

Andarist commented Nov 1, 2021

@joeldenning thank you for your contribution and for bearing with me. I've made a TODO item for this week to land this - it's very close to that, just some nits and one last look at this. I can take care of all of that myself so we don't end up 🏓 each other any longer.

@Andarist Andarist enabled auto-merge (squash) November 7, 2021 17:41
@Andarist Andarist merged commit 2b49c39 into changesets:main Nov 7, 2021
@Andarist
Copy link
Member

Andarist commented Nov 7, 2021

Ufff, managed to land this before the end of the week 😉

Thank you very much for your contribution ❤️

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

3 participants