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

Changelog no longer includes critical BREAKING CHANGE information #275

Closed
mbargiel opened this issue May 20, 2022 · 12 comments
Closed

Changelog no longer includes critical BREAKING CHANGE information #275

mbargiel opened this issue May 20, 2022 · 12 comments

Comments

@mbargiel
Copy link
Contributor

mbargiel commented May 20, 2022

The change to lerna-changelog seems like it resulted in hiding very important information from the changelog, notably about breaking changes: they have simply disappeared from the changelog. It now looks simple, trivial and riskless to upgrade from 0.1.0 to 0.2.1, when in fact there's a handful of breaking changes.

Compare the old changelog: https://github.com/raszi/node-tmp/blob/c8823e549280e11697a510184a69b63bf5bfef7a/CHANGELOG.md

With the new changelog: https://github.com/raszi/node-tmp/blob/7ae22ed2d56c10d425a66e99fe8bc10c925442e6/CHANGELOG.md

As you may note, PRs for issues #156, #207 and #218 all introduced breaking changes and yet are not listed in the new changelog.

@silkentrance
Copy link
Collaborator

Thank you for reporting this.

The problem here is that for the changelog we require a breaking label on the PRs, which does not exist.
Instead I introduced the compatibility label... sigh

@silkentrance
Copy link
Collaborator

silkentrance commented Aug 25, 2022

Since fixing the changelog makes no sense, we will leave it as it is.

Instead, we will revert these breaking changes (#268, #278).

@silkentrance
Copy link
Collaborator

And, my bad, too, there was never a PR for these breaking changes in the first place...

@mbargiel
Copy link
Contributor Author

Actually, the breaking changes I was referring to were not related to the quote-stripping behaviour but rather other changes that weren't documented clearly in the changelog as breaking. Since the changelog is generated, I see how this can't be adjusted now (that's why I stay away from glorified commit list dumps generate changelogs) 😄 But I saw the README has a rather prominent section titled An Important Note on Compatibility, maybe just list any missing breaking changes there? It's an opportunity to also include a note on how to fix the incompatibility and/or why this was done.

@mbargiel
Copy link
Contributor Author

You can even add a ⚠️ icon to those entries mentioning they are not documented as breaking in the changelog but will cause issues. E.g.

⚠️ Undocumented breaking change ⚠️

@silkentrance
Copy link
Collaborator

Yes, we could do that.

@silkentrance
Copy link
Collaborator

I have added additional information to the README. See the section on 'previously undocumented...'

https://raszi.github.io/node-tmp/

@silkentrance
Copy link
Collaborator

@mbargiel please report here any breaking changes that I missed.

@silkentrance
Copy link
Collaborator

Except for mkstemp like behaviour, where in the past you could pass in an absolute path that then included the template file name. Absolute paths must be passed explicitly via the tmpdir option, only.

@mbargiel
Copy link
Contributor Author

@silkentrance The 0.2.0 changelog (prior to automatic its generation) also listed these two things:

drop support for node version < v8.17.0

BREAKING CHANGE

node versions < v8.17.0 are no longer supported.

https://github.com/raszi/node-tmp/issues/216

BREAKING CHANGE

SIGINT handler has been removed.
(...)

These could be captured in the Compatibility section of the readme under 0.2.0, for people who are on that/considering upgrading to that version. (For some reason, not everyone would go to the latest version, I guess? Also for traceability...)

Consider for instance an entry:

Version 0.2.0
Since version 0.2.0, all support for node version < v8.17.0 has been dropped.

SIGINT handler has been removed. Users must install their own SIGINT handler and call process.exit() so that tmp's process exit handler can do the cleanup. A simple handler would be: process.on('SIGINT', process.exit);

@mbargiel
Copy link
Contributor Author

I think you also don't need to have a dedicated section on "Previously Undocumented Breaking Changes". You could just move those 3 breaking changes to the Compatibility section, but add **_Reverted in 0.2.2_** maybe? That would save you to have this extra section and you can keep just a single "Compatibility" section to list any future breaking change that isn't captured in the changelog (if any).

Anyway, those are just ideas 😄 The section you added is also clear.

@silkentrance
Copy link
Collaborator

@mbargiel The SIGINT handler was a mistake in the first place. So it was a bug and not a breaking change, as it prevented user defined SIGINT handlers from working. And it should never have been recorded as a breaking change in the first place. (still learning :)

As for the newly introduced section: I do think that this is relevant to have in a separate section as it caused some unwanted hickups in the past and the users that had been thrown off by those changes need to know, explicitly.

Thank you for your kind and valuable input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants