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

Config.changelog set to false, still writes CHANGELOG.MD files after exiting prerelease #896

Closed
sdirosa opened this issue Aug 1, 2022 · 2 comments · Fixed by #900
Closed

Comments

@sdirosa
Copy link
Contributor

sdirosa commented Aug 1, 2022

Affected Packages

As far as I can tell the logic of this is in @changesets/apply-release-plan.

Problem

We have recently switched to changesets in our monorepo (very happy with it!) but we don't want to use the changelog feature (yet).

I've set "changelog": false in our .changeset/config.json. This is the full config:

{
  "$schema": "https://unpkg.com/@changesets/config@2.1.0/schema.json",
  "changelog": false,
  "commit": false,
  "fixed": [["@objectiv/*"]],
  "linked": [],
  "access": "public",
  "baseBranch": "main",
  "updateInternalDependencies": "patch",
  "ignore": []
}

When I exit a prerelease and run yarn changeset version to apply my changesets, I also end up with changelog files being created in all the packages of the monorepo. I am not sure if this happens also without pre, I haven't tested that, as we only do prerelease flows.

Some superficial investigation

From a quick check on the source code it seems the getNewChangelogEntry function correctly checks whether changelogs have been disabled to skip some initial logic, but then proceeds to return a list of packages with changelogs anyway.

Later in the code, the bit of code that processes the list of finalisedRelease entries doesn't check the config either. Instead it validates the presence of the changelog attribute. E.g. changelog && changelog.length > 0.

Unless I am completely off (I only spent a few minutes reading the code), since finalisedRelease is partially based on the output of getNewChangelogEntry, I'd guess that most probably the changelog attribute is always present and filled with something.

It's not a big deal for us, we can run a find and delete the changelogs before committing. That said, I think quite a bit of logic runs to process changelogs and it could be entirely skipped when that setting is set to false.

Proposed solution

Would be really cool if setting changelog to false in the config.json would result in no changelogs being generated at all, just package.json updates.

A quick fix could be to either skip getNewChangelogEntry when config.changelog is false, or double check whether changelogs have been disabled when processing the list of finalisedRelease and skip the changelog logic there.

I'd be very happy to try and fix this up, or test it if necessary.

@Andarist
Copy link
Member

Andarist commented Aug 2, 2022

I've checked the PR that introduced schema.json and it seems that false was always a valid value there:
https://github.com/changesets/changesets/pull/132/files#diff-d3013abc54530a5bedc30c2f340c829e5dc49923a70bdf4cc469180bf047cb38R18-R21
Types added as part of this PR also suggest so:
https://github.com/changesets/changesets/pull/132/files#diff-e71f2026e90f1ed0c00f8711eb855f0a8810f33dbf1491f5da9c97cdd513e5b4R64
and even more so the getNormalisedChangelogOption function from this PR:
https://github.com/changesets/changesets/pull/132/files#diff-2eafc75a9015704b121dff9585728616f562e1625da9a53e44a86cacb94ae619R16-R21

The validation error message doesn't specify it as allowed though:
https://github.com/changesets/changesets/pull/132/files#diff-2eafc75a9015704b121dff9585728616f562e1625da9a53e44a86cacb94ae619R48-R54

The same~ message is still in the codebase now and thus it should be adjusted to make it clear that false is also a valid option.

Since this PR only declares the new config shape etc but doesn't really implement any features I've also rechecked the code at its point in time and it seems that back then there was an updateChangelog option to control what you want:

if (config.updateChangelog) {
logger.log("Updating changelogs...");
// Now update the changelogs
const changelogPaths = await updateChangelog(releaseObj, config);
if (config.commit) {
for (const changelogPath of changelogPaths) {
await git.add(changelogPath, cwd);
}
}
}

This makes me believe that the original intention to allow false was to, in fact, turn off the changelog generation.

By digging a little bit deeper we can also see that at some point with the new config it was working as intended because the "default" getChangelogFunc was returning an empty string:

let getChangelogFunc: ChangelogFunction = () => Promise.resolve("");

and because it stayed used when config.changelog === false then the CHANGELOG files were never getting updated here:
if (changelog && changelog.length > 0) {
await updateChangelog(changelogPath, changelog, name, prettierConfig);
touchedFiles.push(changelogPath);
}

So this has accidentally regressed in this PR when getChangelogFunc became getChangelogFuncs:
https://github.com/changesets/changesets/pull/147/files#diff-81efc39f0a283b4d7521917c7d32688cc909b4c27820807c7fc5e554b9ab37c7R134-R137

TLDR: I'm confident to classify this as a bug and I would gladly accept a PR fixing this 😉

@sdirosa
Copy link
Contributor Author

sdirosa commented Aug 2, 2022

Thanks for the quick reply and in-depth analysis @Andarist! It really helps to see the history of the code and where this regressed.

By the way, the documentation also describes this intention of using false as a way of disabling changelog generation:

Where it states:

If it is false, no changelogs will be generated.

If I can make some time I'll definitely give this a shot.

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 a pull request may close this issue.

2 participants