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

RFC: Semantic Versioning for TypeScript Types #730

Merged
merged 41 commits into from
Apr 19, 2022

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Mar 23, 2021

Summary

Introduce a policy for semantic versioning for TypeScript types, which accounts for both the details of TypeScript's structural type system and the need to absorb breaking changes in the TypeScript compiler.

This RFC proposes a definition of Semantic Versioning for managing changes to TypeScript types, including when the TypeScript compiler makes breaking changes in its type-checking and type emit across a “minor”release. (Note that this RFC addresses only type checking and type emit, not the “transpilation”mode of the TypeScript compiler.)

While this RFC is being authored in the context of Ember.js’ adoption of TypeScript as a first-class supported language, its recommendations are intentionally general, in hopes that these recommendations can be widely adopted by the broader TypeScript community.

Rendered text

Notes

  • For discussion of whether Ember should adopt TypeScript at all, see RFC: Official TypeScript support #724.
  • For discussion of Ember's own support policy (for core packages and for supplemental packages), see the forthcoming RFC on the subject, for which this is required foundational material. In this thread, please focus on this design for SemVer for types.
  • If you're a non-Ember user but have with an interest in this proposal, welcome! We'd love your feedback and input as we work to make a design here that benefits not only Ember but the entire TypeScript ecosystem!

Background

This is adapted from the work done at typed-ember/ember-cli-typescript#1158 – with significant changes made to the recommendations around the supported compiler version policy (recommending two alternative approaches) and adding details around strictness, module interop, and more.

This RFC proposes a definition of [Semantic Versioning][semver] for
managing changes to TypeScript types, including when the TypeScript
compiler makes breaking changes in its type-checking and type emit
across a “minor”release.(Note that this RFC addresses *only* type
checking and type emit, *no t* the “transpilation”mode of the TypeScript
compiler.)

[semver]: https://semver.org

While this RFC is being authored in the context of Ember.js’ [adoption
of TypeScript as a first-cl ass supported language][RFC], its
recommendations are intentionally general, in hopes that th ese
recommendations can be widely adopted by the broader TypeScript
community.

[RFC]: emberjs#724
@chriskrycho
Copy link
Contributor Author

Thanks so much for the detailed feedback, @RyanCavanaugh! Deeply appreciated, and will update this to account for those in the days ahead!

- author and publish its types with `strict: true` and `noPropertyAccessFromIndexSignature: true` in its compiler configuration


## How we teach this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emberjs/learning-team-managers definitely want to have the input of the Learning Core Team here. Not sure if this information should be a separate repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input very welcome!

The reason I suggested to put it in a separate repo is that the goal is (explicitly!) for it not to remain an Ember-specific policy and design, much as semver.org is used by many projects. We need this, for our adoption process; but many others would benefit from it as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It strikes me that another way this process could go is to develop the repo independent of Ember first then make an RFC for adopting this practices within Ember. However, it’s going to be important for the project to have at least one major adoptee and if it can’t even get Ember to adopt then it’s pretty much a non-starter. So to me, that makes a good case for doing it the way this RFC has proposed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s also worth noting that this spec is mainly for package developers not end users. All end users really want to know is “we won’t break your types willy-nilly.” Of course, having an actual spec with details that they can read is useful, but I strongly suspect that most users won’t really care about the details. If we do it right, things will just work. Where the spec matters a lot more is for those making a package who actually have to make sure they don’t break stuff! All that to say, I don’t think there really needs to be a detailed explanation on the main Ember docs beyond “we adhere to TS SemVer.”

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in terms of how we teach this, I like the idea of it being a separate repo when it's finished. Many successful specs are living documents that have changes over time, and I think it makes sense for those standards conversations to have a home that is not mixed in with conversation about other unrelated things (and vice versa). We could keep it as an RFC entry, but I think as a standalone it has a better chance of influencing the broader JS ecosystem to adopt the same things, versus creating their own RFC version of this with modifications.

An alternate approach is to make the separate repo now, and then link to it in this RFC as a proposal to adopt that document. That would make the Ember relationship a bit clearer, but I think the quality of feedback and iteration cycle really benefits from the full text being in the RFC. We just need to make sure there are clear, bold links to the other repo in this RFC later on.

Copy link
Contributor

@jeremy-w jeremy-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great. I’m still somewhat confused by “user constructible”, and I’m curious where enums fit in. The practical, detailed, non-normative appendices are a big help.

text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
text/0730-semver-for-ts.md Outdated Show resolved Hide resolved
@chriskrycho
Copy link
Contributor Author

Thanks for the feedback, @jeremy-w! (😬 at all the typo/wordo fixes!) I'll be integrating the suggestions etc. later this week. Great callout on enums (const and regular); definitely a miss there. I believe enums, like classes, emit both type and value symbols—with the caveat that const enum gets converted to plain values at build time, and so doesn't have the possibility of runtime conflicts with the normal settings.

User-construct-ability requires a definition, I think, as well as a single choice of spelling. 😆 (Autocorrect converts "constructable" to "constructible," so it looks like that's what we'll be using.) As a brief summary for folks following this, though, the idea there is to provide package authors a way of publishing a type definition that's useful to end consumers while giving them the "out" of saying that the type is only constructible by the package itself, for example via a "factory"-style function.

As a motivating example from Ember, the Transition type is a public type, but is only allowed to be constructed by Ember—specifically by the router. It's therefore public API, and removing fields or methods from it or changing them to a different type would be breaking. However, because only Ember's routing layer is allowed to construct a Transition, it's (mostly) safe to e.g. add a non-optional field to it. If a user has "constructed" a Transition, any breakage they experience at that point is on them. (I say "mostly" because I'm still chewing on Ryan's point about assignability safety for that case in general, but I think that the user-constructibility makes it safe? But I need to mull on it further.)

@jeremy-w
Copy link
Contributor

That helps clear up the constructibility question a lot. “Client code should not attempt to directly create an instance of this type, but can work with instances provided by the library using their public API.” Explicitly stating the changes considered non-breaking is probably needed to avoid issues with breaking derived types as well, eg breaking union discrimination or extension with a conflicting type as raised in other comments.

@jenweber
Copy link
Contributor

I am a huge fan of having the SemVer definitions/expectations in writing somewhere. I don't have enough hands-on experience authoring public library types to comment on the specifics, but as an end user of TS, I can see how this is helpful. Thanks for writing it up!

@tjjfvi
Copy link

tjjfvi commented Apr 7, 2021

It seems to me like many/most of these rules could be summed up by:

  • Non-user-constructible types can be narrowed
  • Non-user-consumable types can be widened
  • Exports cannot be removed

And I'd be proponent of the following rules (not including them makes it almost impossible to change anything without causing a "breaking change"), though they don't always agree with what has been specified in the document:

  • typeof someValue is not a user-constructible type
  • Exports can be added

@chriskrycho
Copy link
Contributor Author

I like those first three much as summaries, @tjjfvi! Thank you for putting it so clearly.

As for the others, I think the typeof one could be repurposed/rephrased very slightly as a rule of thumb for consumers (though with the more specific spec text for the details). Adding exports is trickier for the reasons currently spelled out in the doc: it’s pretty easy to come up with cases where adding exports can break consumers. That said, a “don’t give things the same name as an import” rule of thumb for consumers would help here, and I can see an argument for simplifying the rules around value-only and type-only exports to simply say that unless explicitly specified by the package authors (thinking of cases where one would do this is weird, though) the name and type export are both considered “taken” if the other is exported.


(For lookers-on, I’ll be returning to this next week or the week after. I was busy wrapping up the quarter the last few weeks and am off this week!)

@tjjfvi
Copy link

tjjfvi commented Apr 7, 2021

Adding exports is trickier for the reasons currently spelled out in the doc: it’s pretty easy to come up with cases where adding exports can break consumers.

Any new export can break a consumer:

// myWrapperAroundLib.ts
export * from "lib"
export * from "./myUtils"

// myUtils.ts
export const something = 5;

If lib adds a something export, that will clobber the something export from myUtils in myWrapperAroundLib.ts and cause an error.

@chriskrycho
Copy link
Contributor Author

Ah, that’s a fair point, and one we need to explicitly address one way or another! I’ll mull on it further; at first blush I think that re-exports are an area the current spec design basically doesn’t account for at all!

@wagenet
Copy link
Member

wagenet commented Apr 7, 2021

Without having given it too much consideration, would it make sense to just ban export *?

@tjjfvi
Copy link

tjjfvi commented Apr 7, 2021

import * can also have a similar problem

@chriskrycho
Copy link
Contributor Author

An update: I've been thinking on and talking with folks about especially the problems raised by Ryan, and I have the outline of some changes which I expect to land mid-to-late next week here, namely addressing the way that inference and mutability interact; that covers one subset of the issues Ryan addressed (though there are a couple others outstanding there, which neither I nor my regular collaborators here have yet come up with a robust solution for).

Co-authored-by: Jeremy W. Sherman <[email protected]>
@chriskrycho
Copy link
Contributor Author

chriskrycho commented May 21, 2021

I've been mulling on the open points here for a while and it finally clicked for me this past weekend (I may just be slow!) that all of the inference + mutability issues ultimately come down to variance (as do many of the points I noted in the draft). For example, on function return types, I originally wrote this:

a function (including a class method) which returns a more specific ("narrower") type, for example if it previously returned number | undefined and now always returns number—since all user code will continue working and type-checking

However, @RyanCavanaugh correctly pointed out that this would fail in this scenario:

const arr = [libFunc()];
arr.push(undefined);

The problem here is that Array<T> should be invariant (as e.g. the C♯ docs on variance note), because it's mutable/writable—and a ReadonlyArray<T> would be perfectly safe in this scenario. The same goes for the let example. The usual rule of thumb is that read-only data types (“sources”) can be covariant, while write-only data types (“sinks”) can be contravariant, but mutable data types (which operate as both sources and sinks) must be invariant.

The best solution I've come up with here so far is:

  1. Document this! We can teach the mental model.
  2. Provide tooling which people can use to be safe about this.
    • For package authors, it should be possible to use API Extractor to analyze changes in the output and identify where variance comes into effect here.
    • For consumers of a package, we can provide a lint rule which handles this correctly (one can imagine a @typescript-semver/plugin etc.). Although many users will opt out of it for convenience, this will at least allow folks who want to be extra safe to "do the right thing", e.g. by declaring a given type as read-only, or as sufficiently-wide type to handle it.

I'll be integrating a discussion of variance into the text to account for this.

@chriskrycho
Copy link
Contributor Author

We talked about this at today’s Framework Core team meeting and are moving it forward into Final Comment Period!

This was just a mental flub: it was always intended to be the latter,
and I just repeatedly typo'd it.
@tjjfvi
Copy link

tjjfvi commented Apr 12, 2022

noPropertyAccessFromIndexSignature -> noUncheckedIndexedAccess

Ah, that makes a lot more sense

@tjjfvi
Copy link

tjjfvi commented Apr 14, 2022

I am an active member of the TypeScript community, and given (emphasis mine):

(While this RFC is being authored in the context of Ember.js’ adoption of TypeScript as a first-class supported language, its recommendations are intentionally general, in hopes that these recommendations can be widely adopted by the broader TypeScript community.)

I feel that feedback from the TypeScript community at large is valuable.


I have significant concerns about the current content of the proposal, and though it may be workable for Ember, I think the current provisions would be unworkable for many TypeScript libraries.

Consider the imaginary package frobnicate, maintained by a friendly guy named Fred. It's a simple package, on the surface. Fred has chosen to follow Ember.js's Semantic Versioning for TypeScript Types in frobnicate. This is the source code of [email protected]:

export type FrobnicateOptions = {
  intensity: number,
  rng: () => number,
}

export function frobnicate(options: FrobnicateOptions) {
  // ...
}

This is perfectly reasonable code. The most notable thing about it is that it exports FrobnicateOptions, but this is usually considered good practice, as it allows users to use it in cases such as:

const frobnicateOptions: FrobnicateOptions = {
  intensity: 5,
  rng: () => Math.random() ** 2,
}

If FrobnicateOptions is not exported, users must resort to something like Parameters<typeof frobnicate>[0], which is rather unattractive.

Soon, Fred gets a feature request – in the rng function, a user wants to be able to know some degree of state, without having to track it manually. This is a simple fix, Fred realizes; he can simply pass a number indicating how many times the function has been called before in this frobnication pass.

export type FrobnicateOptions = {
  intensity: number,
  rng: (callNumber: number) => number,
}

export function frobnicate(options: FrobnicateOptions) {
  // ...
}

Now, someone can create an rng such as i => Math.random() * i.

Fred is happy with this change, and seeks to release it as [email protected].

However, to his horror, he realizes that this would be a breaking change:

A change to any object type (user constructible or not) is breaking when:

  • a non-readonly object property's type changes in any way

Source


This would usually not be considered a breaking change, as () => number is assignable to (x: number) => number, and thus no type errors would occur in constructing code.

However, the current proposal treats all types as user-consumable, and so this change would break user code such as:

const options: FrobnicateOptions = {
  intensity: 5,
  rng: () => Math.random() ** 2,
}

options.rng()

Because now the type of options.rng expects a parameter.


(Note: I see mention of user-consumability in some of the above comments; was that once part of the document, and later removed, or was it proposed and then never implemented?)

I propose the following additions to the RFC:

A new concept, user-consumability.

Exported values from a library are, by default, user-consumable but not user-constructible, unless specifically noted otherwise in their type.

User-consumability of a type entails the following permissions:

  • Reading properties of a value of the type
  • Writing properties of a value of the type, if they are not readonly
  • Calling a value of the type, if it is a function type

The terms user-constructible and user-consumable refer to ways of constructing and consuming the types without going through the library; for example, user-constructible types allow object literal construction, and user-consumable types allow property access.

In particular, "constructing" and "consuming" values by way of user-consumable library functions is always allowed.

Specifically:

  • Users can "consume" a non-user-consumable type by passing it to a user-consumable function exported from the library
  • Users can "construct" a non-user-constructible type by getting it from a user-consumable function exported from the library
  • A user-consumable function may take non-user-constructible arguments, and its return type may be non-user-consumable.
  • A user-constructible function may take non-user-consumable arguments, and its return type may be non-user-constructible.

(Terms other than user-consumability and user-constructibility may be worthwhile, as they are rather close to each other.)

@tjjfvi
Copy link

tjjfvi commented Apr 14, 2022

Also, in the vein of requiring noUncheckedIndexedAccess, it may make sense to also require exactOptionalPropertyTypes

@chriskrycho
Copy link
Contributor Author

@tjjfvi this is excellent feedback—exactly what we’ve been hoping for! 💙 Do you mind opening a Discussion for both of these on the repo for the spec? 🙏🏼

@tjjfvi
Copy link

tjjfvi commented Apr 14, 2022

@chriskrycho Certainly; I didn't realize that that repo existed 😅. I've opened semver-ts/semver-ts#3, semver-ts/semver-ts#4, and semver-ts/semver-ts#5.

@chriskrycho
Copy link
Contributor Author

Per discussion at last week’s Framework Team meeting, we accepted this and have pointed to https://www.semver-ts.org as the now-authoritative form of the spec (it was just waiting on me to add that text for us to merge it)! 🎉

@chriskrycho chriskrycho merged commit e233d72 into emberjs:master Apr 19, 2022
@chriskrycho chriskrycho deleted the semver-for-ts branch April 19, 2022 01:35
wagenet added a commit that referenced this pull request Aug 24, 2023
Advance RFC #730 "Semantic Versioning for TypeScript Types" to Stage Ready for Release
wagenet added a commit that referenced this pull request Aug 24, 2023
Advance RFC #730 `"Semantic Versioning for TypeScript Types"` to Stage Released
ef4 added a commit that referenced this pull request Sep 15, 2023
Advance RFC #730 `"Semantic Versioning for TypeScript Types"` to Stage Recommended
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants