Skip to content
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

[RFC] What constitutes a breaking change? #249

Closed
mike-north opened this issue Aug 21, 2018 · 15 comments
Closed

[RFC] What constitutes a breaking change? #249

mike-north opened this issue Aug 21, 2018 · 15 comments
Labels
RFC Requests for Comments

Comments

@mike-north
Copy link
Contributor

mike-north commented Aug 21, 2018

Because of #246 / DefinitelyTyped/DefinitelyTyped#28282 we've hit a point where we have to think about how SemVer, library version and the Ember/TS version support policy interact.

TL;DR - a feature was added to TS 3.1 nightly builds which caused @types/ember to break. TypeScript regards their change as non-breaking, and thus appropriate for a minor release.

However -- to get things working in @types/ember, we've had to use TypeScript 2.8 features. In a sense, we've definitely broken things for someone who's using TypeScript 2.4. There are a variety of packages that depend on @types/ember that will need to be bumped up before we can merge in the fix

I believe some options to discuss are

(1) Bump everything up, and encourage everyone to start using TypeScript 2.8 (this is what the PR currently does)
(2) Take the current types, put them in a /v2 folder, and alter all the dependencies such that they point to /v2. After nothing depends on the v2 types, merge in the fix as the first commit of a v3 release series

@mike-north
Copy link
Contributor Author

Part of this discussion should include this

{ name: 'typescript', target: 'latest' },
{ name: '@types/ember', target: 'latest' },
{ name: '@types/rsvp', target: 'latest' },
{ name: '@types/ember-test-helpers', target: 'latest' },
{ name: '@types/ember-testing-helpers', target: 'latest' },
{ name: '@types/ember__test-helpers', target: 'latest' },

We're currently installing other libraries into apps with ^floating target versions. If our types (and this library) are choosing another versioning system in place of SemVer, we should at least ensure that we don't set anyone up for unexpected breakage. This might mean pinning everything in place at the moment of install.

@dfreeman
Copy link
Member

Has anyone seen examples of how the @types packages for other popular libraries deal with this kind of situation? We certainly have a fair amount of complexity in our types, but I can't imagine we're the first to hit this issue of being forced to choose between matching the source library version and keeping semver.

In the absence of a perfect option, I'd lean on established conventions from elsewhere, if we can find any.

@chriskrycho
Copy link
Member

See also previous discussion around our notion of semver etc. in #190. We mostly didn't cover types there, just TS itself and Ember and our own stability aims, but it's worth noting that the discussion does indeed exist.

@mike-north
Copy link
Contributor Author

@chriskrycho as you indicate, this issue didn't cover versioning ambient types much. I do like the sentiments you all express there, many of which can be applied to types

My interest is to prevent/mitigate one thing: A developer cannot make a seemingly innocuous change in dependencies (i.e., a yarn upgrade without changing a version constraint in their package.json) and break their app. This is one thing that SemVer solves, but we can also get the same benefit by pinning dependencies in place in the installation blueprints so (by default) consumers won't run into upgrade breakage.

In theory everything that's supported in Ember 2.0.0 should remain supported in Ember 2.999.999. If you're stuck on Ember 2.0.0, having types that describe newer minor versions ahead of you should not be a big problem. If we decide to match the major version of Ember, any @types/ember@^2 should work with ember@2.0.0, and @types/ember@latest should track ember@latest

If we decide to match the Ember major release digit, we still have patch/minor to play with. Minor releases typically indicate new features -- as I see it, this not really a thing in the world of ambient types. We are just striving to perfectly describe a piece of code -- there no features, only fixes.

I'd like us to consider the following

2  // Tracking the ember@2 releases
.
1  // Breaking change since @types/ember@2.0.x
.
2  // Non-breaking change since @types/ember@2.1.0

This will let us do the following things

  • Make it clear to consumers, which major version of ember the types are designed to work with
  • Install @types/ember-* stuff with ~target dependencies and yarn upgrade with confidence, knowing nothing will break
  • Adopt the latest TypeScript versions with confidence, incrementing the minor version every time we change the supported TS range, or make some other sort of breaking change

@dfreeman
Copy link
Member

dfreeman commented Aug 22, 2018

A developer cannot make a seemingly innocuous change in dependencies (i.e., a yarn upgrade without changing a version constraint in their package.json) and break their app.

Agreed that this is the key criterion we need to meet. It's similarly a big deal for addons that test with unlocked dependencies in CI (which is the default), since they'll always get the latest in-range version of their @types packages.

I like the scheme you're proposing @mike-north — being able to track the source package's major version (be it ember-source, ember-data, whatever) while still having the flexibility to distinguish breaking from non-breaking changes to the types themselves seems like it ticks all the boxes here.

In this world, what's the ideal behavior of the ember-cli-typescript installation blueprint? For the @types packages it adds, we'd certainly want to add the ~ constraint, but does ember g ember-cli-typescript update you to the latest versions of those packages, or only add them if they're not already present?

@mike-north
Copy link
Contributor Author

mike-north commented Aug 22, 2018

In this world, what's the ideal behavior of the ember-cli-typescript installation blueprint? For the @types packages it adds, we'd certainly want to add the ~ constraint, but does ember g ember-cli-typescript update you to the latest versions of those packages, or only add them if they're not already present?

Going back to the principle expressed by @chriskrycho in #190

[something is] a breaking change if someone runs ember install ember-cli-typescript@latest and sees the diff and accepts it, a previously-type-checking app will no longer type-check.

I see an update of e-c-ts@latest, running the install blueprint and accepting all diffs as "bring this up to latest release", which could definitely be a breaking change. There's no way around this, especially when you consider that our afterInstall hook may do completely different things in the future. This is consistent with the "reinstall the latest version of this dependency" experience people are used to in a wide range of languages and package mangers.

We shouldn't be encouraging developers to re-run the install blueprint in any addon as a "non-breaking upgrade" mechanism

With that in mind, here's how we may want to answer your question

what's the ideal behavior of the ember-cli-typescript installation blueprint?

It should

  • Log a single warning if any @types/ember(-*)@^latest dependency is found, telling developers that it's the SemVer equivalent of "@types/ember": "*" including a link to an explanation of the versioning policy (maybe this warning could be silence-able via an ember-cli-build.js config)
  • Install @types/ember(-*)@~latest for any missing dependencies, and any existing ~target dependencies

This should result in the following experience

  • Running e-c-ts@latest and accepting everything, updates you to the latest stuff (potentially breaking, consistent with any other addon)
  • Explicitly stating "@types/ember": "^3.0.0" is still possible, but results in warnings by default
    • You can get rid of this warning if you want sneaky breaking changes to enter your app, but you have to deliberately opt-out
  • The stuff e-c-ts installs will be set up so you don't accept breaking changes unless you change your package.json
  • Tools like greenkeeper and dependabot will work nicely as new breaking @types/ stuff is released

@mike-north
Copy link
Contributor Author

If there are no objections, I'm going to document the above plan and apply it to the pending @types/ember change

@dfreeman
Copy link
Member

I'm 👍 on the plan. The generator behavior we're after might be a little finicky (I'm not aware of a single-pass way to both resolve the latest version of a package and set a ~ dependency on it), but definitely workable.

@mike-north
Copy link
Contributor Author

mike-north commented Aug 24, 2018

@dfreeman part of closing this out will involve writing a vanilla node module to expose utilities to help make decisions about explicitly upgrading a dependency (or not). I want us to write a crazy number of unit tests around a pure function to ensure the blueprint does what we want it to.

@chriskrycho
Copy link
Member

100% in favor of the “write a crazy number of tests” plan and 👍🏼 on all of this more generally. Thanks for taking this on!

@mike-north
Copy link
Contributor Author

mike-north commented Aug 25, 2018

The behavior we've agreed to above is now packaged into a npm module, which does all the analysis and comparison and will feed us a POJO with info to act on

https://github.com/mike-north/typever

{
  lib: { name: 'commander', target: '^2.17.1', version: '2.17.1' },
  types: {
    recommendedTarget: '~2.12.2',
    name: '@types/commander',
    target: '^2.12.2',
    version: '2.12.2'
  },
  result: {
    compatibility: 'warn',
    reason:
      'Type library target of "^2.12.2" will allow your app to take in breaking changes.\nThis is the SemVer equivalent of { "@types/commander": "*" }',
    suggestion:
      'Update package.json with dependency { "@types/commander": "~2.12.2" }'
  }
}

@mike-north
Copy link
Contributor Author

It feels like we have consensus here. I'll close this in a couple of days and open PR(s) to put the plan into action

@mike-north mike-north changed the title What constitutes a breaking change? [RFC] What constitutes a breaking change? Sep 6, 2018
@mike-north mike-north added the RFC Requests for Comments label Sep 6, 2018
@chriskrycho
Copy link
Member

@mike-north what's the status on this one? I don't recall seeing the typever stuff land here at all, and if so that's obviously fine; we've all been super busy 😂 – I just noted it as I was going back through issues for basic triage.

@mike-north
Copy link
Contributor Author

mike-north commented Mar 12, 2019

We've been manually enforcing the TypeVer strategy. Haven't had a chance to do the work to consume the library in this package yet, but that's the easy part tbh

@chriskrycho
Copy link
Member

Resolved over the course of 2022–2023 via the combination of:

Thanks again to everyone who helped make that happen. 🎉

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
RFC Requests for Comments
Projects
None yet
Development

No branches or pull requests

3 participants