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

feat(version): add support for changelog #2358

Closed
wants to merge 6 commits into from
Closed

Conversation

akphi
Copy link

@akphi akphi commented Jan 11, 2021

What's the problem this PR addresses?
This PR aims to introduce a fairly basic changelog generation feature for version plugin
Fixes #1510

How did you fix it?
I followed the guidance listed in #1510 (comment) and referred to the approach and the UX of changesets.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

TODOs - will find some time to work on these, I'm pretty slammed this week 😭

  • Support another format for a changeset (markdown instead of yaml) - right now I just add a changelog field to the existing yaml file.
  • Generate and add changelog entries to CHANGELOG.md file.

Things to consider - @arcanis

  • 💬 Support multi changelog entries with respect to each workspace (more discussion needed - see [Feature] Changelog support in version (?) plugin #1510)
  • Do we keep changelog when there is no accepted changelog (edge case: first there was some accepted change, they added some changelog and then they changed their mind and make the changeset unaccepted)?
  • Do we need to show any placeholder text when the changelog input is empty?

@akphi
Copy link
Author

akphi commented Jan 11, 2021

@arcanis The more I work on this the more I realize we should modify (or have a flag for) yarn version check -i so that we can scan the differences between upstream/master and the current branch and bring up interactive mode. Right now, we only based on changes in git working directory; this means the changeset is only for current the commit. But a lot of time I just code, commit, and want to make one commit for changelogs. Also, this is conflicting with the behavior of yarn version check -i where it allows to edit available changeset if available.

So, I audaciously went ahead and tried to use --octopus for merge-base check so that now as long as your PR differs from upstream/master version check -i will show something. See the doc for merge-base --octopus, discussion between different merge-base algorithms and commit 7c36d51. We need to modify this:

const {stdout: mergeBaseStdout} = await execUtils.execvp(`git`, [`merge-base`, `HEAD`, ...ancestorBases], {cwd: root, strict: true});
const hash = mergeBaseStdout.trim();

As I said, I could imagine if this is intentional, it means the changeset is per commit. But would be nice to have one changeset per PR. Let me know if I completely misunderstood things here 😄 But eager to hear what you think!

@akphi akphi changed the title feat: add support for changelog for version plugin feat(version): add support for changelog for version plugin Jan 11, 2021
@akphi akphi changed the title feat(version): add support for changelog for version plugin feat(version): add support for changelog Jan 11, 2021
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Looks nice! Do you know if Ink supports a way to display an input box with a series of dots, instead of taking three rows? I quickly checked but didn't find anything 🤔 cc @vadimdemedes

Comment on lines +1 to +2
releases:
"@yarnpkg/plugin-version": minor
Copy link
Member

Choose a reason for hiding this comment

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

No changelog? 😛

Copy link
Author

Choose a reason for hiding this comment

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

@arcanis just in the draft mode now, I will add it when I add support for changelog generation next. Will gladly be the first try-er of this feature 😄

@akphi
Copy link
Author

akphi commented Jan 11, 2021

Looks nice! Do you know if Ink supports a way to display an input box with a series of dots, instead of taking three rows? I quickly checked but didn't find anything 🤔 cc @vadimdemedes

I don't know what you meant by a series of dots. I thought to give it a clear visual distinction, I would use some sort of bordered box, and so I looked at their guide on border styles

@arcanis
Copy link
Member

arcanis commented Jan 12, 2021

I meant something a bit like this: https://manaflair.github.io/mylittledom/demo/#complex-form

This way it would take a bit less vertical space. Not critical, but if there's a way ... 😄

@arcanis
Copy link
Member

arcanis commented Jan 12, 2021

Right now, we only based on changes in git working directory; this means the changeset is only for current the commit. But a lot of time I just code, commit, and want to make one commit for changelogs.

Just saw this - I'm not sure I follow. We fetch the list of changed files between master and the current HEAD, but that list includes any commit between the two of them (otherwise I wouldn't get the right result when running the edit, as you mentioned, but it works fine). The workflow you mentioned should already work 🤔

@akphi
Copy link
Author

akphi commented Jan 12, 2021

I meant something a bit like this: https://manaflair.github.io/mylittledom/demo/#complex-form

This way it would take a bit less vertical space. Not critical, but if there's a way ... 😄

@arcanis awesome work there! Of course, I wouldn't mind aiming for the best possible way. I myself don't link the current border-box because when you resize it looks bad (until you type the next character - or have some useEffect on resize - we'll see about that).

I have looked at the doc for ink and it doesn't seem to me that there's a good way to achieve what you asked for (a very small comment is ........... is a bit unpleasant to the eyes and if the user end their sentences in and actual period, that's confusing, so we need to give those fillers .......... a more subtle color, such as grey).

Doing that way, we also have to think about text-wrapping. Looks like for @manaflair/mylittledom, text-wrapping is disabled and we only show truncated text. ink <Text> support this, but ink-text-input <TextInput> does not.

Also, while we're here, there's a problem with multi-line changelog. Since we block on enter key right now to save action, we can't really use the return key, on top of that, there's a Node issue that prevents us from using alt/option + enter to create a new line 😭 - we might want to revisit our decision to use enter as hotkey to trigger save

Anyway, I'm all over the place here. I'll see what I can do: I will highly likely skip this in first-pass.


tl;dr

If it's too time-consuming to go the ......... way, I will try to just limit the width of the changelog input for now. See image:

Screen Shot 2021-01-12 at 8 47 56 AM

If vertical space is of your concern, we can use borderStyle={\classic`}sinceborderStyle={`round`}` gives too little vertical padding. Like this:

Screen Shot 2021-01-12 at 9 16 48 AM

@akphi
Copy link
Author

akphi commented Jan 12, 2021

Just saw this - I'm not sure I follow. We fetch the list of changed files between master and the current HEAD, but that list includes any commit between the two of them (otherwise I wouldn't get the right result when running the edit, as you mentioned, but it works fine). The workflow you mentioned should already work 🤔

@arcanis Just to be on the same page (because I'm so new to contributing as well), my workflow is:

  1. Create a fork of akphi/berry of yarn/berry
  2. Clone akphi/berry onto my Mac. Set yarn/berry as upstream remote
  3. Work on akphi/berry master branch locally. Commit directly on master here
  4. Push changes to my akphi/berry master branch
  5. Create PR and pull changes from akphi/berry master to yarn/berry master.

So in this scenario:
HEAD = local akphi/berry/master
master = local akphi/berry/master
origin/master = origin akphi/berry/master
yarn/berry/master = upstream/master

According to the doc for [merge-base] when we call git merge-base HEAD master origin/master upstream/master (which is our default changesetBaseRefs)

const {stdout: mergeBaseStdout} = await execUtils.execvp(`git`, [`merge-base`, `HEAD`, ...ancestorBases], {cwd: root, strict: true});
const hash = mergeBaseStdout.trim();

We will get HEAD as the merge-base, instead of the commit where I fork yarn/berry, which is what I think I would want. To get that commit, I would need to call the above command with --octopus flag to get the common ancestor of all of my refs.

This means that on my local, if I have made several commits, but my current git working directory is clean right now, when I call yarn version check -i I will get nothing. What I really want is for yarn version check -i to account for all of those commits that belongs to my PR.

@arcanis
Copy link
Member

arcanis commented Jan 12, 2021

  • Work on akphi/berry master branch locally. Commit directly on master here
  • Push changes to my akphi/berry master branch
  • Create PR and pull changes from akphi/berry master to yarn/berry master.

I think that's the problem. The idea (which perhaps should be better documented!) is that when working on a feature you first create a feature branch. Typically in this repo we call them username/subject, for instance mael/esbuild. Then when you run yarn version check, we check what are the changes in the current feature branch compared to master. As a result, if you run this command on master, you'll only get the changes that haven't been committed, hence the behavior you've noticed.

To sum up, a better experience would be to:

  • git clone your repo
  • git checkout -b my/feature-name to create a new feature branch
  • Make a few commits, as many as you want
  • yarn version check -i which will collect the version upgrades
  • Want to make a few other commits? No worry, keep doing it as you want.
  • You can also keep running yarn version check -i, it'll keep using the same file for the rest of the feature branch lifetime.
  • When you're ready, make a PR and merge into master.

@akphi
Copy link
Author

akphi commented Jan 12, 2021

@arcanis I see, so to accommodate for the workflow that I mentioned (i.e. develop on fork master branch directly), I suppose we simply change changesetBaseRefs: upstream/master. I will revert that commit. Thanks!

@akphi
Copy link
Author

akphi commented Jan 19, 2021

Just a quick status update. I will come back to this in a week or two due to a tight schedule. Sorry to anyone who's waiting on this for the delay.

@fmal
Copy link

fmal commented Apr 4, 2021

@akphi looks wonderful - would love to use this feature one day in my projects. Any chance you find time to finalize this?

@akphi
Copy link
Author

akphi commented Apr 6, 2021

@fmal I totally forgot about this 😭. My motivation was to make this work better with changesets and I managed to tinker a bit to just make changesets work with Yarn@2 so I lost sight of this issue. But I will find time to make this happen in hopefully a week or 2. Sorry for keeping everyone waiting!

@emilio-martinez
Copy link

Hi @akphi 👋 thanks for tackling this — much needed feature. So is your latest approach to just leverage changesets under the hood?

@akphi
Copy link
Author

akphi commented Apr 27, 2021

Hi @akphi 👋 thanks for tackling this — much needed feature. So is your latest approach to just leverage changesets under the hood?

@emilio-martinez The main motivation for me to start all of this is that I cannot use changesets with yarn@2 as changesets does not understand the workspace protocol so during publish phase, it ends up publishing a bad package.json file to NPM. Luckily, there's a workaround there. So this feature is no longer as much needed by me anymore.

Regardless, I'll find some time to come back to this. I think since the last time I was discussing with @arcanis I got a pretty good understanding of what it takes.

@akphi
Copy link
Author

akphi commented Oct 3, 2022

I'm going to close this PR here. We have moved on to use changesets for our project so I have lost my initial motivation to work on this feature. Really sorry about that. I will close this to make clear my intention of leaving it for someone else to pursue.

I'm still following the original thread #1510 and still believe being able to align yarn changelog with tools like changesets is a sensible move. As such, if someone else wants to pick this back up and build on top of my existing code, I'm happy to discuss what I have in mind.

@akphi akphi closed this Oct 3, 2022
@jj811208
Copy link
Contributor

jj811208 commented Oct 4, 2022

Thanks to @akphi's efforts, I think the requirement of this feature seems to be more clear after this PR.

I've been working on this issue for a few days now, but since I didn't join the discussion, my understanding of all this comes from #1510, there's no consensus on this feature yet, right?

I have some insights and would like to ask you if I am on the right track🥸.


1 PR = 1 version file mapping do not change (at least in the first iteration)

Implement an input box in yarn version check -i so that users can explain what their change is about. For the prototype an single input box is enough. Later on we may want one input per workspace, but not in the first iteration.

After working on it, I think one input box is not enough, because version files may only be partially iterated over in Yarn.

For example:

---
"@yarnpkg/cli": minor
"@yarnpkg/core": minor
---

Does something new

cd packages/yarnpkg-core && yarn version apply

---
"@yarnpkg/cli": minor
- "@yarnpkg/core": minor
---

Does something new

We have no way of knowing which package this changelog is from, so I think each package should have its own changelog,

here comes the problem, I think there are multiple input boxes in the "yarn version check -i", which seems to be bad for DX, also, Ink doesn't seem to have a good textarea component.

So I want "yarn version check-i" to not pay attention to updating the changelog.

Change the generated version files to be closer from what Changeset generates. In particular, switch to markdown (but no need for a full markdown parser, I think it would be overkill). This will allow us to share more tools between our projects, which would let us use their GitHub bot, etc.

I think the conversion to markdown is a very good practice, because markdown is quite suitable for manual editing changelog.

However, if we consider this matter together with the first and second points, how to use the ecosystem of changeset will be a difficult problem.

change logs production and formatting do not belong to a package manager,

I think it makes sense that maybe yarn shouldn't care about this and just go manage everything in package.json (dependencies, licenses, versions...etc.)

But as a loyal yarn user, I think it would be great if yarn version deferred could support changelog🤪.

So I can convince myself that yarn is a package manager, and package manager is related to package version, and package version is related to changelog, so yarn and changelog are related.

Maybe it should be maintained by a third party? I'm not so sure, But given the context, I would expect it to be a plugin independent of yarn version plugin,

partly to make it easier for it to be extracted from the yarn repo (if needed in the future),

partly to avoid huge conflicts (the yarn team seems to be rewriting the yarn version plugin #4867)

In addition, it seems to be easier to integrate changeset's ecosystem when it is independent.


The tentative idea is

adding some hooks to yarn version plugin.

1 PR = 1 version file mapping.
1 version file = 0...* changelog mapping.

changelog is manually edited by the user.

only works when version deferred


I'm really not very good at English and I'm still learning, so I'm sorry if I caused you any difficulty in reading

@akphi
Copy link
Author

akphi commented Oct 5, 2022

@jj811208 Whew, that's quite a lot. I'm afraid I only have answers for a few points though.

Regarding points 1 and 2, I think when user wants to create a new changeset, showing only 1 input box makes sense. Overall, I often find myself in 2 situations:

  1. For a change, I need to add multiple changesets, each changeset corresponds to 1 package and has different messages
  2. For a change, I want to use the same changeset for multiple packages and with the same message

With that, I feel like showing one input box is the only way to address these needs. If one wish to create multiple changesets like in case (1) feel free to call the command multiple times

Regarding point 4, I'm not sure about what you said, I think one of the requirements is when call yarn version, we will create a commit (or staging changes) which bumps the version in packages' package.json and also, remove the changesets and aggregate them into CHANGELOG.md file(s). As such, I'm not sure if it makes sense to keep those separate.

Anyhow, I think you have some nice ideas there, and some I can't really answer 😅 , so for the benefit of everyone, let's bring this discussion back to #1510 if possible. Thanks!

@jj811208
Copy link
Contributor

jj811208 commented Oct 5, 2022

Thank you for your reply!

I feel a lot of uncertainty with this initial idea, and I'm worried that it will be communicated to too many people.

But when I'm ready I will!

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.

[Feature] Changelog support in version (?) plugin
5 participants