-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ci: updated global changelog #5328
base: main
Are you sure you want to change the base?
Conversation
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
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.
If this is an urgent automation, I think this approach can work as a temporary fix with a few hardening updates to how it's written but long-term we need to be handling this from the Changesets tooling (https://github.com/changesets/changesets/blob/main/docs/modifying-changelog-format.md#writing-changelog-formatting-functions)
scripts/add-global-changelog.js
Outdated
const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
const repoUrl = 'https://github.com/adobe/spectrum-web-components'; | ||
|
||
const pkg = JSON.parse( |
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.
A best practice to keep code scoped is to wrap them in a main function and call the function at the end of the file. I think it's a good idea to maintain that best practice here that we see in our other scripts as well.
scripts/add-global-changelog.js
Outdated
const pkg = JSON.parse( | ||
fs.readFileSync(path.resolve(__dirname, '../package.json'), 'utf-8') | ||
); | ||
const newVersion = pkg.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.
I think you can skip this abstraction since newVersion is only used in the following line. We also need a check for if the package doesn't load to throw that warning. You're assuming here that pkg.version exists.
const prevTag = execSync('git tag --sort=-creatordate') | ||
.toString() | ||
.split('\n') | ||
.filter(Boolean) | ||
.find((tag) => tag !== newTag); |
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 needs more robust failure captures. I recommend if you're going to use exec for this, separate the command execution (exec is notoriously flaky in node scripts so you need to account for it failing) from the string parsing. Check that exec returned a string and then run the split, etc.
if (!prevTag) { | ||
console.error('No previous tag found.'); | ||
process.exit(1); |
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.
I'm trying to think about what information I would need to debug this error. At the age of this project, there's no change that there aren't previous tags to be found so maybe we want this error to tell us why the exec command couldn't return a value we were expecting? Maybe this should log the git tag command output?
process.exit(1); | ||
} | ||
|
||
const date = new Date().toISOString().split('T')[0]; |
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.
const date = new Date().toISOString().split('T')[0]; | |
const date = new Date().toLocaleDateString('en-CA', { | |
year: 'numeric', | |
month: '2-digit', | |
day: '2-digit', | |
})); |
This should return the result you're wanting without having to do inline array parsing (which can sometimes lead to invalid results or fail when the array isn't in the format we're expecting).
} | ||
|
||
const date = new Date().toISOString().split('T')[0]; | ||
const compareUrl = `${repoUrl}/compare/${prevTag}...${newTag}`; |
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.
Is prevTag allowed to be a pre-tag or is there a requirement that prevTag must be a semver version? It seems like it must be one of the semver releases (not the betas for example) so maybe we can add a comment to that effect?
const commitLogs = execSync(`git log ${prevTag}..HEAD --pretty=format:"%s|%h"`) | ||
.toString() | ||
.trim(); |
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 looks like it's returning the commit logs but not the changelog content. Is that what we're wanting to add to the global changelog? It seems like the commit history is less useful now that we've migrated to changesets.
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 a great point where I would want everyone's opinion on. I don't think only a summary of the change is sufficient for the users to check what changes went along. I want to keep the CHANGELOG to follow the same pattern as we were doing during lerna which I feel the users still wants.
|
||
// Skip if nothing relevant | ||
if (!features.length && !fixes.length) { | ||
console.log('🚫 No new feat() or fix() commits to add.'); |
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.
console.log('🚫 No new feat() or fix() commits to add.'); | |
console.log('🚫 No new feat() or fix() commits to add.'); |
Since no new features or fixes isn't necessarily a failure of the script, should we format this more like a success message that it ran successfully but with no changes?
commits.forEach(({ message, hash }) => { | ||
const typeMatch = message.match(/^(feat|fix)\(([^)]+)\):\s*(.+)/i); | ||
if (typeMatch) { | ||
const [, type, scope, description] = typeMatch; | ||
const entry = `- **${scope}**: ${description} ([\`${hash}\`](${repoUrl}/commit/${hash}))`; | ||
if (type === 'feat') { | ||
features.push(entry); | ||
} else if (type === 'fix') { | ||
fixes.push(entry); | ||
} | ||
} | ||
}); |
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.
I think this can serve a temporary fix but we should really be using the changesets tooling to create this content from it's new source (which is not the commit messages): https://github.com/changesets/changesets/blob/main/docs/modifying-changelog-format.md#writing-changelog-formatting-functions
I think the challenge with this as a sustainable approach is that less and less useful data is present in commit messages and the real value for customers now lives in the changesets files.
scripts/add-global-changelog.js
Outdated
|
||
fs.writeFileSync( | ||
changelogPath, | ||
`${newEntry.trim()}\n\n${existingChangelog}`, |
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.
Wouldn't this push the # Change log
heading down to below the latest update?
Description
This PR introduces a dual changelog generation setup in our monorepo:
✅ Per-package changelogs via @changesets/changelog-github
✅ Custom global changelog appended to the root CHANGELOG.md using a Yarn 4-compatible ESM script
Changes
Configured .changeset/config.json to use @changesets/changelog-github for individual packages.
Added a custom ESM script (scripts/add-global-changelog.js) to:
Read newly generated changelogs from the .changeset output
Format and append only feat: and fix: commits to the root CHANGELOG.md
Automatically add diff links (e.g., 1.4.0)
Updated package.json scripts to include:
Related issue(s)
CAHNGELOG.md
Motivation and context
Since Changesets doesn’t support a global changelog out of the box, this is a custom script to bring that adds the experience back.
How has this been tested?
Run yarn changeset to create a changeset file
Run yarn version to bump versions and generate changelogs
On postversion, the global script appends new release notes below the root changelog header
Test Plan
Verified correct changelog output for multiple changesets
Confirmed global script doesn't overwrite existing CHANGELOG.md
Validated diff links and date stamps
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.