search

raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
732 stars 93 forks source link

Changelog no longer includes critical BREAKING CHANGE information #275

Closed mbargiel closed 1 year ago

mbargiel commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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

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

silkentrance commented 1 year ago

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

mbargiel commented 1 year ago

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) :smile: 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 commented 1 year ago

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 commented 1 year ago

Yes, we could do that.

silkentrance commented 1 year ago

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

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

silkentrance commented 1 year ago

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

silkentrance commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 :smile: The section you added is also clear.

silkentrance commented 1 year ago

@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.