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

A list of suggested update to TypeScript's lib #49773

Closed
19 of 27 tasks
graphemecluster opened this issue Jul 4, 2022 · 9 comments · May be fixed by #49855, #50450, #50451 or #50452
Closed
19 of 27 tasks

A list of suggested update to TypeScript's lib #49773

graphemecluster opened this issue Jul 4, 2022 · 9 comments · May be fixed by #49855, #50450, #50451 or #50452
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Milestone

Comments

@graphemecluster
Copy link
Contributor

graphemecluster commented Jul 4, 2022

lib Update Request

While working on uhyo/better-typescript-lib, I've found couples of issues in the built-in lib. While reflecting some of the changes there will cause compatibility issues and they are not included in the list below, others are worth reporting because either they are serious or they are not breaking changes. Here is the list:

es2015.collection.d.ts

  • WeakMapConstructor: The type of entries should change from readonly [K, V][] | null to readonly (readonly [K, V])[] | null to keep it consistent with MapConstructor

es2015.core.d.ts

  • {Array<T>, ReadonlyArray<T>}.find:
    • The this parameter of the first parameter in the first overload is incorrectly typed void, which should be removed
    • Missing documentation for the second overload
  • ArrayConstructor.from: The first parameter should be renamed source to keep consistency across multiple overloads from other lib files. In es2015.iterable.d.ts it's written iterable: Iterable<T> | ArrayLike<T> and that's misleading. It's also named source in the spec text. (The documentation should be modify to an array-like or iterable object too to keep it the same across all files)
  • NumberConstructor.{isFinite, isInteger, isNaN, isSafeInteger}: Per the spec, the return type can be changed to the type predicate number is number (Excluded unless Suggestion: one-sided or fine-grained type guards #15048 resolves)
  • ObjectConstructor.assign: All parameters but the first should be made writable ({ -readonly [P in keyof T]: T[P] }).
  • ObjectConstructor.{getOwnPropertySymbols, setPrototypeOf}: 🟠 The first parameter should extends {}. See the es2017.object.d.ts section below
  • String.normalize: 🟠 Per the spec, the second overload should be removed to prevent a runtime error. The first parameter should then be made optional because it's defaulted to "NFC".

es2015.iterable.d.ts

  • {ArrayConstructor, %TypedArray%Constructor}.from: See the es2015.core.d.ts section above

es2015.reflect.d.ts

Crossed out unless @uhyo thinks them worth changing and gives some illustrations.

  • Reflect.construct: newTarget can be widen to any
  • Reflect.getPrototypeOf: The return type can be narrowed to object. It's impractical to get a null here
  • Reflect.setPrototypeOf: proto can be widen to any

es2015.symbol.wellknown.d.ts

es2017.object.d.ts

es2019.array.d.ts

  • {Array<T>, ReadonlyArray<T>}.flatMap: The This type parameter should be removed to keep consistency with other array prototype methods in the current library

es2020.bigint.d.ts

  • %TypedArray%.{every, find, findIndex, some}: The return type of the first parameter can be widen to any to keep consistency with other array prototype methods
  • %TypedArray%Constructor.from: Missing documentation for the second overload. Also see the es2015.core.d.ts section above

es2021.string.d.ts

  • String.replaceAll: missing { [Symbol.replace] } overloads. It should be defined exactly the same as String.replace (The documentation should be modify to A string or RegExp search value too to keep it the same across all files)

es2022.object.d.ts

  • Object.hasOwn: Per the spec, o can be widen to {}

es5.d.ts

  • ObjectConstructor.{getPrototypeOf, getOwnPropertyDescriptor, getOwnPropertyNames}: 🟠 The first parameter should extends {}. See the es2017.object.d.ts section above
  • {CallableFunction, NewableFunction}.apply: Missing documentation for the second overload
  • {CallableFunction, NewableFunction}.bind: Consider rewrite it like what we've done
  • {Array<T>, ReadonlyArray<T>}.every: 🟠 The return type of the first overload can be typed this is { [K in keyof this]: S } to support tuples. Unlike other solutions, this change is small enough that it should not break anything. It does affect some important tests, so a separate PR will be opened.
  • ReadonlyArray<T>.map: 🟠 The return type can be typed { -readonly [K in keyof this]: U } to support tuples. Will be in the follow-up PR
  • Array<T>.map: 🟠 The return type can be typed { [K in keyof this]: U } to support tuples. Will be in the follow-up PR
  • {Array<T>, ReadonlyArray<T>}.{reduce, reduceRight}: Missing documentation for the second overload
  • %TypedArray%.{find, findIndex}: The return type of the first parameter can be widen to any to keep consistency with other array prototype methods
  • %TypedArray%Constructor.from: See the es2015.core.d.ts section above
🟠 Breaking Changes (non-exhaustive)

If the TypeScript Team is fine with certain of these changes, I am happy to open a PR for it.

Thanks for reading till the end. That is all.


P.S. I am not sure if I shouldn't put all of the above into a single issue, but opening numerous issues to reflect all of these suggestions will be too many for me. I wonder how many PRs would be comfortable to the Team and how should I split it if I'm gonna work on this.

@MartinJohns
Copy link
Contributor

  • NumberConstructor.{isFinite, isInteger, isNaN, isSafeInteger}: Per the spec, the return type can be changed to the type predicate number is number

That would be a bad change. See this comment: #39090 (comment)

@graphemecluster
Copy link
Contributor Author

@MartinJohns Thanks for pointing that out. I've just crossed that item out. Are there any other inappropriate changes? My apologies if there're still any.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 4, 2022

I didn't look thoroughly through it, just a few points here and there that raise eyebrows, like

Consider rewrite it by the union to intersection hack like what we've done (See https://stackoverflow.com/questions/50374908)

The UnionToIntersection type has been called an ill idea by the TypeScript team several times, and has no official support. You definitely won't see that in a standard library.

See this comment: #41857 (comment)

We don't support anything that involves UnionToIntersection; this is an ill-defined and will frequently produce "surprising" results.

Besides that it's very unclear which proposed changes would be breaking changes.

@graphemecluster
Copy link
Contributor Author

@MartinJohns Thanks again. I've marked some of them as breaking changes. I wonder if the changes to {Array<T>, ReadonlyArray<T>}.{every, map} are also considered breaking.

@fatcerberus
Copy link

ObjectConstructor.setPrototypeOf: 🟠 The first parameter should extends {}.

Surely it should be object? {} basically means "anything object-coercible" which includes primitives, but it doesn't make much sense to set the prototype of a primitive number, for example. While it might not throw, it's not going to do anything useful, either.

@graphemecluster
Copy link
Contributor Author

@fatcerberus Yes. Originally I did put an ”at least“ in that line, but later I realized that consistency should be kept across all Object prototype methods. Moreover currently it is typed any and suddenly making it object may cause inappropriate breaks in current codebases. Although altering it to {} is still a breaking change, I considered this appropriate even if it breaks current codebases because it may cause runtime errors.

@Josh-Cena
Copy link
Contributor

I do see your doubts on these, but I'd still like to point out:

  • Reflect.construct: newTarget can be widen to any

It has to have a [[Construct]] internal slot, so Function seems like the best type we can have

  • Reflect.getPrototypeOf: The return type can be narrowed to object. It's impractical to get a null here

Reflect.getPrototypeOf(Object.create(null))?

  • Reflect.setPrototypeOf: proto can be widen to any

Reflect.setPrototypeOf({}, "1") is a TypeError.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experience Enhancement Noncontroversial enhancements labels Jul 5, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jul 5, 2022
@RyanCavanaugh
Copy link
Member

Some of these seem OK and some are a little bit dicier. We can take a PR and evaluate case-by-case -- please be sure to include testcases that demonstrate what's being fixed.

@wchargin
Copy link

Object.hasOwn: Per the spec, o can be widen to {}

This is checked and the issue is closed, but it is not actually fixed because #50451 was never merged. Can we reopen the issue or at least un-check the item to avoid confusion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment