Navigation Menu

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

Jump to bottom

Google feedback on TypeScript 3.5 #33272

Closed
evmar opened this issue Sep 5, 2019 · 22 comments
Closed

Google feedback on TypeScript 3.5 #33272

evmar opened this issue Sep 5, 2019 · 22 comments
Labels
Discussion Issues which may not have code impact

Comments

@evmar
Copy link
Contributor

evmar commented Sep 5, 2019 •
edited

We recently upgraded Google to use TypeScript 3.5. Here is some feedback on the upgrade.

(For background, recall that Google is a monorepo of billions of lines of code. We use a single version of TypeScript and a single set of compiler flags across all teams and upgrade these simultaneously for everyone.)

We know and expect every TypeScript upgrade to involve some work. For example, improvements to the standard library are expected and welcomed by us, even though they may mean removing similar but incompatible definitions from our own code base. However, TypeScript 3.5 was a lot more work for us than other recent TypeScript upgrades.

There were three main changes in 3.5 that made it especially painful. (The other changes were also required work, but these three are worth some extra discussion.) We believe most of these changes were intentional and intended to improve type checking, but we also believe the TypeScript team understands that type checking is always just a tradeoff between safety and ergonomics.

It is our hope that this report about TS 3.5 as applied to a large codebase will help the TypeScript team better evaluate future situations that are similar, and we make some recommendations.

Implicit default for generics

This was the headline breaking change in 3.5. We agree with the end goal of this change, and understand that it will shake up user code.

Historically when TypeScript has introduced type system changes like this, they were behind a flag.

Suggestion: Using a flag here would have allowed us to adapt to this change separately from the other breaking changes in 3.5.

The main way this failed is in circumstances where code had a generic that was irrelevant to what the code did. For example, consider some code that has a Promise resolve, but doesn't care about what value the Promise to resolves to:

function dontCarePromise() {
  return new Promise((resolve) => {
    resolve();
  });
}

Because the generic is unbound, under 3.4 this was Promise<{}> and under 3.5 this becomes Promise<unknown>. If a user of this function wrote down the type of that promise anywhere, e.g.:

const myPromise: Promise<{}> = dontCarePromise();

it now became a type error for no user benefit.

The bulk of churn from this generics change was in code like this, where someone wrote a {} mostly because it was what the compiler said without really caring what type it was.

One common concrete example of this don't-care pattern are the typings for the d3 library, which has a very complex d3.Selection<> that takes four generic arguments. In the vast majority of use cases the last two are irrelevant, but any time someone saves a Selection into a member variable, they ended up writing down whatever type TS inferred at that time, e.g.:

mySel: d3.Selection<HTMLElement, {}, null, undefined>;

The 3.5 generics change means that {} became unknown simultaneously in almost every interaction with d3.

Suggestion: Our main conclusion about specifically d3 is that the d3 typings are not great and need some attention. There are some other type-level issues with them (outside of this upgrade) that I'd like to go into more, but it's not relevant to this upgrade.

Another troublesome pattern are what we call "return-only generics", which is any pattern where a generic function only uses it in the return type. I think the TypeScript team already knows how problematic these are, with lots of inference surprises. For example, in the presence of a return only generic, the code:

expectsString(myFunction());

can be legal while the innocent-looking refactor

const x = myFunction();
expectsString(x);

can then fail.

Suggestion: We'd be interested in seeing whether TypeScript could compile-fail on this pattern entirely, rather than picking a top type ({} or unknown). Users are happy to specify the generic type at the call site, e.g. myFunction<string>() but right now the compiler doesn't help them see when they need it. For example, maybe the declaration myFunction<T>(...) could always require a specific T to be inferred, because you can always write myFunction<T=unknown>() for the case where you are ok with a default.

One other common cause of return-only generics is a dependency injection pattern. Consider some test framework that provides some sort of injector function:

function getService<T>(service: Ctor<T>): T;

where Ctor<T> is some type that matches class values. The intended use of this is e.g.

class MyService { … }
const myService = getService(MyService);

This works great up until MyService is generic, at which point this again picks an arbitrary <T> for the return type. The problem here is that we pass the MyService value to getService, but then we get back the MyService type, which needs a generic.

One last source of return-only generics that we discovered is that the generic doesn't need to be in the return type. See the next section.

filter(Boolean)

TypeScript 3.5 changed the type of the Boolean function, which coerces a value to boolean, from (effectively)

function Boolean(value?: any): boolean;

to

function Boolean<T>(value?: T): boolean;

These look like they might behave very similarly. But imagine a function that takes a predicate and returns an array filter, and using it with the above:

function filter<T>(predicate: (t: T) => boolean): (ts: T[]) => T[];
const myFilter = filter(Boolean);

With the 3.4 definition of Boolean, T is pinned to any and myFilter becomes a function from any[] to any[]. With the 3.5 definition, T remains generic.

We believe this change was intentional, to improve scenarios like this.

The RxJS library uses a more complex variant of the above pattern, and a common use of it creates a function composition pipeline with a filter(Boolean) much like the above. With TS 3.4, users were accidentally getting any downstream of that point. With TS 3.5, they instead get a generic T that then feeds into a larger inference. You can read the full RxJS bug for some more context.

One of the big surprises here is that everyone using RxJS was getting an unexpected any at this point. We knew to look for any in return types, but now we know that even if you accept an any in an argument type, via inference this can cause other types to become any.

Suggestion: A more sophisticated definition of Boolean, one that removed null|undefined from its generic, might have helped, but from our experiments in this area there are further surprises (search for "backwards" on the above RxJS bug). This was also not mentioned in the list of breaking changes in 3.5. It's possible its impact was underestimated because it disproportionately affects RxJS (and Angular) users.

Set

In TypeScript 3.4,

const s = new Set();

gave you back a Set<any>. (It's actually a kind of amusing type, because in some sense almost everything still works as expected -- you can put stuff in the set, .has() will tell you whether something is in the set, and so on. I suspect this might be why nobody noticed.)

TypeScript 3.5 made a change in lib.es2015.iterable.d.ts that had the effect of removing the any, and the generic change described above made it now infer unknown.

This change ended up being tedious to fix, because the eventual type errors sometimes were pretty far from the actual problem. For example, in this code:

class C {
  gather() {
    let s = new Set();
    s.add('hello');
    return s;
  }
  use(s: string[]) { … }
  demo() {
    this.use(Array.from(this.gather()));
  }
}

You get a type error down by the Array.from but the required fix is at the new Set(). (I might suggest the underlying problem in this code is relying on inference too much, but the threshold for "too much" is difficult to communicate to users.)

Suggestion: we are surprised nobody noticed this, since it broke our code everywhere. The only thing worth calling out here is that it seems like nobody made this change intentionally -- it's not in the breaking changes page, and the bug I filed about it seems to mostly have prompted confusion. The actual change that I think changed what overloads got picked looks harmless. Perhaps the main lesson we learned here is that we needed to discover this earlier and provide this feedback earlier.

PS: It also appears new Map() may have the same problem with any.

Conclusion

I'd like to emphasize we are very happy with TypeScript in general. It is our hope that the above critical feedback is useful to you in your design process for future development of TypeScript.
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 5, 2019 •
edited

new Set()

Regarding new Set(), this is one of those cases where it would be nice to have a type parameter that must always be explicitly specified by the user.

Because there's no way for TS to infer the intended type parameter.

Either that, or have some mechanism to treat the type parameter as an "evolving" type parameter.

Sort of like how,

const arr = [];

Creates an "evolving" array type. As you push elements of different types to the array, the type of arr changes.

Or with,

function x () {}
x.foo = 1;
x.bar = 2;

As you add more properties, the type of x changes

In the above example, your s would be Set<never> or something to start with. Then after .add('hello'), it could become Set<string>

I personally lean more towards being able to force users to explicitly specify a type parameter, especially in cases where it simply cannot be inferred.

new Promise()

Also the same problem as new Set(), really. In my projects, I almost always explicitly specify the type of the promise new Promise<void>(... because TS can't really infer the type parameter of the promise.

And the defaults, {}/unknown, were never useful or correct because I'd normally want void for those "don't care" promises.

Return-only generics

"return-only generics", which is any pattern where a generic function only uses it in the return type.

I'm not sure what this is exactly but I assume it's something like this,

declare function foo<T> () : T;
expectString(foo()); //OK

And the refactor would fail,

const x = foo();
expectStr(x); //Error

This is, again, another instance of TS just not being able to infer the type parameter. And another good reason to add a feature that allows one to force a type parameter to always be explicitly provided.

If it is what I think it is, this return-only generic is actually an anti pattern and a "hidden" cast.

"Don't-care" types

mySel: d3.Selection<HTMLElement, {}, null, undefined>

Speaking of "types-that-exist-but-we-don't-care-about"...

It sounds like an existential type?
I wonder if implementing this will help,
#14466 (comment)

Conclusion

A good workaround for most of these problems is to just have a lint rule to always explicitly specify type params for certain constructors/functions.

And to not use the "return-only generic" anti-pattern. (Or to apply the above proposed lint rule to such instances)

As for don't-care types... I guess it depends but we're all stuck with manually specifying either never, unknown, or any for now. Until existential types get implemented.

Users are happy to specify the generic type at the call site, e.g. myFunction<string>() but right now the compiler doesn't help them see when they need it.

I'm not sure if there's a feature request open for this forcing-users-to-explicitly-specify-a-type-parameter thing. But if there isn't, someone should totally open it.

I'll probably have a look when I get home.

@sandersn sandersn added Meta-Issue An issue about the team, or the direction of TypeScript Planning Iteration plans and roadmapping labels Sep 5, 2019
@fripig fripig mentioned this issue Sep 6, 2019
Open
@estaub
Copy link

estaub commented Sep 6, 2019

FWIW, new Set() and new Promise() might be considered instances of "return-only generic" functions, but no one considers generic container classes to be an antipattern.

Apart from backward compatibility issues (or, "ignoring the elephant in the room"), I'd be happy with much more widespread use of never generic defaults for cases like these.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 6, 2019 •
edited

never itself is a problematic default. When used as the return type of a function, it enables assignment to anything.

A better default (in my opinion) would be an existential type that is never for input positions and unknown for output positions. I'm not sure how much that would impact usability, though.

I wouldn't consider new Set() and new Promise() "return-only generics" because we know they return a value with some shape we can reason about (it has a .then() method, it has a .add() method, etc.). What we don't know is the shape of the elements they contain.

This is safe,

declare function expectStringSet(s : Set<string>) : void;
//OK
expectStringSet(new Set());

declare function expectString(s : string) : void;
//Error
expectString(new Set());

This is safe,

declare function expectStringPromise(s : Promise<string>) : void;
//OK
expectStringPromise(new Promise((resolve) => {
    resolve("str") //OK!
    resolve(42) //Error!
}));

declare function expectString(s : string) : void;
//Error
expectString(new Promise(() => {}));

With a "return-only generic", we can't even reason about the shape of the return type. It's just... A type that we know exists.

This is never safe,

declare function returnOnlyGeneric<T> () : T;

//OK
declare function expectStringSet(s : Set<string>) : void;
expectStringSet(returnOnlyGeneric())

//OK
declare function expectString(s : string) : void;
expectString(returnOnlyGeneric());

//OK
//???
declare function expectNever(n : never) : void;
expectNever(returnOnlyGeneric());;

@DanielRosenwasser DanielRosenwasser added Discussion Issues which may not have code impact and removed Meta-Issue An issue about the team, or the direction of TypeScript Planning Iteration plans and roadmapping labels Sep 6, 2019
@DanielRosenwasser DanielRosenwasser removed their assignment Sep 6, 2019
@evmar
Copy link
Contributor Author

evmar commented Sep 8, 2019

@DanielRosenwasser if you or anyone on your team who was interested read this, feel free to close it; I don't think it's actionable as a bug. (Also lemme know if it was useful or not, and we can do a similar one for 3.6.)

@jkillian
Copy link

jkillian commented Sep 9, 2019 •
edited

(Also lemme know if it was useful or not, and we can do a similar one for 3.6.)

I think it's useful from a community perspective - a lot of other TS users around the world likely hit (or will hit) similar difficulties when upgrading, and having some extra documentation on the issues one might encounter could be tremendously useful.

I've somewhat frequently hit rather arcane-feeling errors with complex react types when upgrading TS and wished that there was a more detailed guide to what's contained in the latest version (though the official release notes are very well done). Perhaps there's some community-curated form of this "TypeScript upgrade challenges and solutions" list that could be created, but even if that never happens, I think your notes are worth posting as a resource to others, so thank you!

@NickHeiner
Copy link

@evmar I imagine y'all needed to use codemods for this... what do you use to codemod? I've been using jscodeshift but find it to be pretty sparsely documented.

@amirburbea
Copy link

I too found the implicit unknown to be problematic. I'd prefer that if the generic doesn't have a
<T = SomeType> style default, and none of the arguments allow inference of a T (similar rules to C#), then require explicit type parameter rather than inserting of unknown.

@DanielRosenwasser
Copy link
Member

Hey @evmar, thanks a ton for writing this up. Having this available publicly is great, and gives us an easy way to return to this discussion, so I especially appreciate that. It sounds like users outside of both Google and Microsoft have gotten something out of it too!

This isn't the first batch of feedback that we've leveraged either. For example, with certain breaks, we've tried to stretch them out over several releases. But for what it's worth, I think that getting this sort of feedback earlier on would be more ideal since we would have realized how impactful each of these changes were (and whether they warranted a flag). While we can err on the side of caution and always introduce flags and the like, that's still more cognitive overhead we'd generally rather avoid.

We've actually expanded our release cadence in 3.6 to be longer in order to encourage users (and larger organizations like Google) to have more time to upgrade and try betas and RCs. I think that the sooner you can upgrade to 3.6 and hopefully the 3.7 beta, the better off the community as a whole will be too. Let us know if there's anything we can do to help there!

@evmar
Copy link
Contributor Author

evmar commented Sep 9, 2019

Thanks @DanielRosenwasser, the slower cadence is definitely helpful. And it really is on us to provide more timely feedback; I totally get that it's not super useful to hear about stuff that are N+2 revs old. We keep getting preempted by other projects but are hoping to catch up soon, and we started on 3.6 basically immediately after we finished 3.5 (see e.g. #33295).

@mheiber mheiber mentioned this issue Sep 10, 2019
Open
5 tasks
@mheiber
Copy link
Contributor

mheiber commented Sep 10, 2019 •
edited

@evmar , I'd be curious about whether you think something like this (if it's feasible) would help next time: #33345

@leemhenson
Copy link

@evmar I'd be really interested to know how you organise your TS in your monorepo. Are you using shared packages, symlinks, project references etc? Probably OT for this thread, but if you could find the time maybe you could post something in #25376 ? Thanks

@amirburbea
Copy link

amirburbea commented Sep 10, 2019 via email

@apaprocki
Copy link

@evmar Are you able to share Google TS adoption as of this 3.5 upgrade milestone (% of TS relative to total JS)? I'm interested in hearing more about others' large-scale adoption efforts/progress.

Since I'm the one asking, I'll go first. At Bloomberg, we have a JS footprint in the broad range of "tens of millions" LoC. Within that footprint, we are now roughly 6% TS adoption by LoC, 8% TS adoption by file count.

@pauldraper
Copy link

I'd be really interested to know how you organise your TS in your monorepo. Are you using shared packages, symlinks, project references etc?

See bazelbuild/rules_nodejs and bazelbuild/rules_typescript. tl;dr Google uses a polyglot build tool Bazel to orchestrate.

@thw0rted
Copy link

thw0rted commented Sep 11, 2019 •
edited

This works great up until MyService is generic, at which point this again picks an arbitrary for the return type. The problem here is that we pass the MyService value to getService, but then we get back the MyService type, which needs a generic.

This bit me not because of DI, but because of a factory pattern. I have a method that takes a Ctor<T> and creates an instance, but T is generic, so a bunch of code that looked like

abstract class Base<T> { ... }
class Sub<T> extends Base<T> { ... }

function create<T>(clazz: Ctor<T>): T { ... }
function consume<T>(instance: Base<T>) { ... }

const x = create(Sub);
consume(x); // Error!

broke for the reasons described in the OP. I "fixed" this by liberally peppering the generic parameters with default unknown but I'd love to see a better approach.

This was referenced Sep 11, 2019
Closed
Closed
@KentoMoriwaki KentoMoriwaki mentioned this issue Sep 12, 2019
Closed
@kdnk kdnk mentioned this issue Sep 12, 2019
Closed
@kolodny
Copy link

kolodny commented Sep 13, 2019

@evmar I'd be really interested to know how you organise your TS in your monorepo. Are you using shared packages, symlinks, project references etc? Probably OT for this thread, but if you could find the time maybe you could post something in #25376 ? Thanks

@rkirov and @bowenni gave on how we use Typescript in Google at TSConf 2018 https://www.youtube.com/watch?v=sjov1k5jexA

@leemhenson
Copy link

@rkirov and @bowenni gave on how we use Typescript in Google at TSConf 2018 https://www.youtube.com/watch?v=sjov1k5jexA

Interesting, but I was really hoping for some discussion about how they do package management and link their local packages together....

@amirburbea
Copy link

amirburbea commented Sep 13, 2019 •
edited

@leemhenson I really think you need to look into lernajs https://lerna.js.org/

I use it at my job, and it's great. I have a root tsconfig in the packages folder with

{
  "files": [],
  "references": [
    { "path": "pkg1" },
    { "path": "pkg2" },
    { "path": "pkg3" }]}

(You can then build all packages with a tsc -b packages/tsconfig.json)

Each project then references my base tsconfig

{
  "compileOnSave": false,
  "compilerOptions": {
    "allowJs": false,
    "allowSyntheticDefaultImports": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "alwaysStrict": true,
    "declaration": true,
    "declarationMap": true,
    "disableSizeLimit": true,
    "emitBOM": false,
    "emitDeclarationOnly": false,
    "emitDecoratorMetadata": false,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "importHelpers": true,
    "incremental": true,
    "isolatedModules": false,
    "lib": ["dom", "es2015", "es2016", "es2017.object"],
    "module": "esnext",
    "moduleResolution": "node",
    "newLine": "LF",
    "noEmitOnError": true,
    "noErrorTruncation": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "noStrictGenericChecks": false,
    "noUnusedLocals": true,
    "preserveConstEnums": false,
    "preserveSymlinks": true,
    "pretty": true,
    "removeComments": false,
    "resolveJsonModule": true,
    "sourceMap": true,
    "strict": true,
    "strictBindCallApply": true,
    "strictFunctionTypes": true,
    "strictNullChecks": true,
    "strictPropertyInitialization": true,
    "stripInternal": true,
    "suppressExcessPropertyErrors": false,
    "suppressImplicitAnyIndexErrors": false,
    "target": "esnext"
  },
  "exclude": ["./node_modules/"]
}

with something like

{
  "compilerOptions": {
    "composite": true,
    "jsx": "preserve",
    "outDir": "lib",
    "rootDir": "src"
  },
  "exclude": ["./lib/", "./node_modules/"],
  "extends": "../tsconfig.base.json",
  "references": [{ "path": "../communication" }, { "path": "../utilities" }]
}

Lerna makes symlinks for all your packages so you still reference the other packages with scoped package names like @my-scope/pkg1

@sod sod mentioned this issue Sep 13, 2019
Closed
@leemhenson
Copy link

@leemhenson I really think you need to look into lernajs https://lerna.js.org/

I know about lerna, I use pnpm for similar. I'm interested in hearing how google do it with monorepo as large as theirs.

@kolodny
Copy link

kolodny commented Sep 13, 2019

@leemhenson The linked video talks about how Google uses Bazel, to build everything from head so there's no versioning concerns https://youtu.be/sjov1k5jexA?t=130 That bit starts at 2:10 on the video

@AnyhowStep AnyhowStep mentioned this issue Sep 15, 2019
Open
@MessagingMoore
Copy link

This difficulty in migrating implicit defaults for generics looks like an argument for #26242 to me.

Specifically, this developer behavior:

... takes four generic arguments. In the vast majority of use cases the last two are irrelevant, but any time someone saves a Selection into a member variable, they ended up writing down whatever type TS inferred at that time

Since in generic types with several parameters, there is not a way to specify one parameter but leave others inferred, developers are forced to specify all of them. (When a default for the type parameter is not viable.) Then if the inferred type changes, even in a way not relevant to that code, that site needs to be updated. Supporting #26242 would at least allow a mechanism for the developer to limit this.

Laziness would then lead to a "pit of success" where developers would leave the last types unspecified rather than blindly copying the inferred type into the code.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Oct 20, 2019 •
edited

The proposals, at the moment, are different.

That proposal is about partial type argument inference for function/method calls.

It seems like what you really want is partial type argument inference for explicit type annotations.

const mySel: d3.Selection<HTMLElement, infer, null, undefined> = /*blah*/;

However, that doesn't really fix anything.

When you hover over the return type of /*blah*/, you will get the full inferred type. Nowhere in this full inferred type is the keyword for "partial inference".

Then, you copy-paste it as an explicit type annotation.

Now, you have an unknown/{} type parameter. Is it the default type parameter value of an unconstrained type parameter or was it explicitly intended?

If it is the default of an unconstrained type parameter, then you want to replace that unknown/{} with infer.

If it isn't the default and is, in fact, explicit, then you want to leave it as unknown/{} and not mess with it.

It just seems like a non-solution, to me.

I can only reasonably see the default constraint type changing once more (if at all) in the future, so it isn't too bad, right?

I'm keeping my fingers crossed for existential types and hoping it'll be the new default constraint type

@mike-north mike-north mentioned this issue Nov 6, 2019
Merged
@danvk danvk mentioned this issue Oct 29, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion Issues which may not have code impact
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests