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

Bloomberg feedback on TypeScript 4.7 Beta #49071

Closed
dragomirtitian opened this issue May 11, 2022 · 5 comments
Closed

Bloomberg feedback on TypeScript 4.7 Beta #49071

dragomirtitian opened this issue May 11, 2022 · 5 comments
Assignees
Labels
Discussion Issues which may not have code impact

Comments

@dragomirtitian
Copy link
Contributor

dragomirtitian commented May 11, 2022

We are in the process of evaluating the impact of upgrading to 4.7 on our codebase. Here some preliminary findings:

# Change Affects Release notes Packages affected Introduced in
1 Detection of weak generic types Type checking Announced 100% #48366
2 Symbol included in keyof T Type checking Not Announced 2%
3 Better always true detection Type checking Not Announced <1%
4 Comments on default exports have moved d.ts emit Not Announced 5%
5 Optional parameter does not on include undefined d.ts emit Not Announced 2% #48605
6 Changes in emitted conditional types d.ts emit Not Announced <1% #48592

Detection of weak generic types

This was reverted by #48923

The generic weak types on constraints has caused all our packages to fail due to an error in a base package. The specific pattern that we ran into is this:

type Spec = {
    props?: string[]
}
type Class<T extends Spec> = { s: Spec }
function cls<T>(s: T):Class<T> { return null! }

Playground Link 4.6 / Playground Link 4.7

This was already reverted but we are implementing fixes based on the error to ensure if this is turned on in the future we will be better prepared.

Symbol is now included in keyof T

This was the biggest source of breaking changes (post reverting weak generic type detection) breaking on the order of 2% of packages. We ran into this when concatenating or interpolating keyof T into a string:

function add<T>(p: keyof T) {
    return "prefix." + p;
}
function message<T>(p: keyof T) {
    return `prefix.${p}`;
}

Playground Link 4.6 / Playground Link 4.7

This new inclusion of symbol appears to be too eager in certain scenarios:

export type Context = {
    V1: { "a": string },
    V2: { "b": string },
}
function path<V extends keyof Context, K extends keyof Context[V]>(v: V, k: K)  {
    // v is treated as string
    // k is not treated as a string, even though it can only ever be "a" | "b"
    return `${v}.${k}`
}

Playground Link 4.6 / Playground Link 4.7

Better detection of always true conditions

This caused 5 errors in 0.5% of packages, 1 of which is definitely an error (an un-awaited promise). The others are method definitions that are checked to exist, mostly in tests so they are probably intentional.

Changes in declaration emit

This version changed declaration emit in several ways that we found to be benign:

  1. Comments on default exports are now moved on the symbol that is exported not the default export statement
  2. Optional parameters no longer emit | undefined on the parameter type if it was not explicitly added (this seems like a improvement)
  3. One declaration map changed for unknown reasons
  4. Changes to how conditional types are inlined. This is due to this PR. This seems like an improvement but we are still investigating the impact on downstream consumers of these packages.
@DanielRosenwasser
Copy link
Member

Thank you for reporting with the results @dragomirtitian! A few questions/comments:

The others are method definitions that are checked to exist, mostly in tests so they are probably intentional

Have you found the experience of squelching these issues to be reasonable?

Comments on default exports are now moved on the symbol that is exported not the default export statement

I would likely consider this a bug if you're willing to file it.

Symbol is now included in keyof T

This is my least favorite unexpected breaking change because anecdotally, nobody cares about symbols. The usual mitigation is to write string & keyof T. How bad was this experience?

@DanielRosenwasser DanielRosenwasser added the Discussion Issues which may not have code impact label May 11, 2022
@DanielRosenwasser DanielRosenwasser removed this from the TypeScript 4.7.1 milestone May 11, 2022
@dragomirtitian
Copy link
Contributor Author

dragomirtitian commented May 12, 2022

@DanielRosenwasser

The others are method definitions that are checked to exist, mostly in tests so they are probably intentional

Have you found the experience of squelching these issues to be reasonable?

It was actually pretty straightforward. In most cases this was a redundant check (for example an assertion function called right before).

Comments on default exports are now moved on the symbol that is exported not the default export statement

I would likely consider this a bug if you're willing to file it.

This is a simple repro of it.

/**
 * Comment
 */
export default {
   test: 0
}

Playground Link 4.6 / Playground Link 4.7

I would hesitate to make this a bug. It's just a change, not sure if it was intentional or not. It does not affect us one way or the other, we just saw it as a difference (we generally check if any build output changes between TS versions and investigate why it did).

We initially feared typedoc might be impacted buy this, but it turns out it wasn't including the comment on the default export in the docs anyway, so I don't see a change there either.

The only noticeable change is that tooltips in VS Code now pick up the comment and surface it, which is an improvement.

Symbol is now included in keyof T

This is my least favorite unexpected breaking change because anecdotally, nobody cares about symbols. The usual mitigation is to write string & keyof T. How bad was this experience?

We mostly encountered this when composing log messages, so we made a runtime change to explicitly convert using String(key). We did this to avoid breaking changes in dependencies, changing .

@Andarist
Copy link
Contributor

This new inclusion of symbol appears to be too eager in certain scenarios:

This case looks interesting. I will take a look at investigating this cause I assume that this is a bug (can't find a reason why it wouldn't be).

@dragomirtitian
Copy link
Contributor Author

@DanielRosenwasser We tested out the 4.7 RC and we didn't find anything new to report. No new breaking changes in our codebase.

@DanielRosenwasser
Copy link
Member

Thank you for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

4 participants