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

Handling unintended breaking changes #1156

Closed
mmarchini opened this issue Jan 25, 2022 · 4 comments · Fixed by nodejs/node#47426
Closed

Handling unintended breaking changes #1156

mmarchini opened this issue Jan 25, 2022 · 4 comments · Fixed by nodejs/node#47426

Comments

@mmarchini
Copy link
Contributor

I think we should talk about breaking changes and deprecation cycles, specifically around accidental or unforeseen breaking changes and how to handle those when they end up in a release. nodejs/node#38924 didn't undergo a deprecation cycle, but it was an accidental breaking change as per our contributing guidelines. This particular case was aggravated by a few factors:

  • The breaking change never went through a deprecation cycle
  • We didn't revert it once we found out it was breaking certain packages
  • Because of tooling issues, the breaking change is not present on changelogs
  • The documentation is still describing the previous behavior

We have a section about unintended breaking changes, but it doesn't go into what should be done when one of those breaking changes lands on a release (it's also only implicit that As an alternative to reverting, the TSC can apply the semver-major label after-the-fact means we're granting a one-time exception for the breaking change to happen). I think we need to clarify what should be done in those scenarios. IMO it's not just about reverting or not, but also taking action to reduce the chances of it happening again in the future (be it adding more tests or adding affected projects to CITGM) as well as ensuring documentations and changelogs reflect the breaking change accordingly. It probably will be a case-by-case thing where the TSC choses from a list of potential actions which one should be taken for each specific breaking change. Furthermore, IMO if the actions cannot be completed in a timely manner, revert should be considered.

I want to hear others thoughts on this, and it doesn't have to be specific to the issue linked. Also, if folks remember other unintentional breaking changes that have landed in the past, please link here so we can inform out decision based on past experiences.

@mcollina
Copy link
Member

It likely happened several times with streams stuff, in the sense that something was shipped in a release, we reverted it but kept it on master.

@tniessen
Copy link
Member

+1 to what @mcollina says. Reverting on existing release lines if it's an unexpected breaking change seems reasonable if done in a timely manner. It might not always be simple to bisect though.

@mhdawson
Copy link
Member

mhdawson commented Feb 3, 2022

@mmarchini , we agreed in last weeks TSC meeting on actions, does still need to be on agenda?

@mhdawson
Copy link
Member

mhdawson commented Apr 5, 2023

Created PR so we can either land some updated guidance or agree to just close this.

RafaelGSS pushed a commit to nodejs/node that referenced this issue Apr 13, 2023
Fixes: nodejs/TSC#1156
PR-URL: #47426
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this issue Jul 6, 2023
Fixes: nodejs/TSC#1156
PR-URL: #47426
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
Fixes: nodejs/TSC#1156
PR-URL: nodejs#47426
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
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.

5 participants