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 missing json-rpc-{engine,middleware-stream} CHANGELOG tag diff links #2008

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 8, 2023

Background

While migrating @metamask/json-rpc-engine and @metamask/json-rpc-middleware-stream into the core repo, some earlier release commits that were untagged in the original repo (and didn't have CHANGELOG entries) were skipped by the tag porting script. These were created manually after the fact, and now they need to be added to the CHANGELOG as well.

Tasks

References

Changelog

N/A

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift force-pushed the 231107-json-rpc-engine-add-missing-changelog-tag-diff-links branch from c28dab9 to 8104e81 Compare November 8, 2023 21:40
@MajorLift MajorLift marked this pull request as ready for review November 8, 2023 21:42
@MajorLift MajorLift requested a review from a team as a code owner November 8, 2023 21:42
@Gudahtt
Copy link
Member

Gudahtt commented Nov 9, 2023

I might be missing something, but are these entries useful? I like that we have the tags now, but omitting these entries from the changelog might make sense given that we don't have any change entries to document

mcmire
mcmire previously approved these changes Nov 9, 2023
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I'm fine with this. It looks like, however, that there are also missing tags for:

@mcmire mcmire self-requested a review November 9, 2023 17:57
@mcmire mcmire dismissed their stale review November 9, 2023 17:58

Dismissing review given Mark's comment. I also ran changelog:validate and it looks like we can just link 5.2.0 directly to the tag instead of a diff.

@MajorLift MajorLift changed the title Add missing json-rpc-engine CHANGELOG tag diff links Add missing json-rpc-{engine,middleware-stream} CHANGELOG tag diff links Nov 9, 2023
@MajorLift MajorLift force-pushed the 231107-json-rpc-engine-add-missing-changelog-tag-diff-links branch from c1f08d0 to 465af60 Compare November 9, 2023 18:17
@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 9, 2023

@Gudahtt autochange-log validation requires that version headings be present in the CHANGELOG for all of the versions in the tag diff links.

I think the tags and tag diff links are definitely adding useful information and shouldn't be removed. Given that, to get rid of the placeholder version headings, I think we'll have to either remove the logic for matching tag diff links with version headings, or add an option to specify a range (or ranges) of versions that we should expect to have empty changelogs because they were never written.

@MajorLift
Copy link
Contributor Author

@mcmire Thanks for finding those commits! I added tags + diff links for those in 13ce5ad, and also found more early tags in json-rpc-middleware-stream.

@MajorLift
Copy link
Contributor Author

I think just keeping the empty version headings might be useful, as they represent the full version history of the package, and the unwritten changelogs is also part of that history.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 9, 2023

We already don't have complete history of packages in our changelogs, as we didn't preserve changelogs from before we migrated metamask-controller to the core monorepo. Personally I'd look in at the tags in the repo if I wanted to dig into older releases, I would never look at the changelog for that. As such I wouldn't find these links useful. It's easy to navigate to the compare pages in GitHub if you know the tags.

I look at changelogs for additional high-level context about what each release consisted of, which is missing here.

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 9, 2023

In that case, seems like the way forward for package migrations would be: a) create the tags for untagged pre-migration releases, but b) not add them to the changelog?

I'm good with that so long as we can expect potential users of the package to look at the changelog as just a high-level overview of version history, instead of an exhaustive source of truth.

Although, it does seem like the empty-changelog tag diff links are equally as useful (or un-useful) as the existing tag diff links. Would it be worth modifying autochange-log so we can avoid potentially confusing users with a partial version history here (without having to add the empty version headings)? Of course, we can always get the complete version history from npm or git ls-remote --tags, so I guess the question is whether our expected audience for the changelog files is internal only or external users as well.

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 9, 2023

I'm good with just closing this PR. So long as the tags are created, I agree that the changes here aren't adding anything meaningful. We could always revisit this if there's confusion about the changelogs in the future. @Gudahtt @mcmire Would that be ok, or is there anything else you'd maybe like to address?

@Gudahtt
Copy link
Member

Gudahtt commented Nov 9, 2023

Maybe we could leave a note about looking at the old archived repo to see the changelog for older releases? If you're concerned about this not being comprehensive, maybe that would be an effective way to fix that

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 9, 2023

The changelogs for these packages were created at json-rpc-engine@5.2.0 and json-rpc-middleware-stream@3.0.0, so all of the versions before that don't have any changelog entries anywhere.

No tag messages or tag diff links in the original repo, since these releases weren't tagged, and it seems no release commit messages, either.

The tag diff compare links we now have in core are our only window into what was changed in these early versions. Including them in the changelog would be nice, but preserving the original starting point for the changelog also makes sense to me.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 9, 2023

What I meant is that we could add this as a change entry:

### Changed
- See here for details: https://github.com/MetaMask/json-rpc-engine/blob/main/CHANGELOG.md

For json-rpc-engine for example

Either for every one of these blank releases, or once with a note about how it applies to all previous releases as well

Edit: Oh I'm sorry, I see, these entries don't exist.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 9, 2023

Given that these releases are ancient history and were never documented, closing this PR and leaving them undocumented would make sense to me.

@MajorLift
Copy link
Contributor Author

Closing as WONTFIX

@MajorLift MajorLift closed this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants