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

fix(compiler): Align return types of ErrImp<E> and OkImpl<T> with Result<T, E> for TS 5.5 #30345

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

nikeee
Copy link
Contributor

@nikeee nikeee commented Jul 16, 2024

Summary

Result<T, E> defines isOk and isErr as type predicates:

  // Returns `true` if the result is `Ok`.
  isOk(): this is OkImpl<T>;
  // Returns `true` if the result is `Err`.
  isErr(): this is ErrImpl<E>;

The implementations just return boolean, which seems to break in TS 5.5:

Running tsc in current main:

tsc
src/Utils/Result.ts:202:3 - error TS2416: Property 'isOk' in type 'ErrImpl<E>' is not assignable to the same property in base type 'Result<never, E>'.
  Type '() => boolean' is not assignable to type '() => this is OkImpl<never>'.
    Signature '(): boolean' must be a type predicate.

202   isOk(): boolean {
      ~~~~

src/Utils/Result.ts:206:3 - error TS2416: Property 'isErr' in type 'ErrImpl<E>' is not assignable to the same property in base type 'Result<never, E>'.
  Type '() => boolean' is not assignable to type '() => this is ErrImpl<E>'.
    Signature '(): boolean' must be a type predicate.

206   isErr(): boolean {


Found 38 errors in 7 files.

Errors  Files
     2  src/HIR/BuildHIR.ts:196
     2  src/HIR/Environment.ts:745
     2  src/ReactiveScopes/CodegenReactiveFunction.ts:290
    14  src/Utils/Result.ts:96
     2  src/Validation/ValidateNoRefAccesInRender.ts:201
     2  src/Validation/ValidateNoSetStateInRender.ts:115
    14  src/__tests__/Result-test.ts:13

How did you test this change?

tsc
# no errors
tsc --version
Version 5.5.3

```ts
  // Returns `true` if the result is `Ok`.
  isOk(): this is OkImpl<T>;
  // Returns `true` if the result is `Err`.
  isErr(): this is ErrImpl<E>;
```

The implementations just return `boolean`, which seems to break in TS 5.5:

Running `tsc` in current `main`:
```
tsc
src/Utils/Result.ts:202:3 - error TS2416: Property 'isOk' in type 'ErrImpl<E>' is not assignable to the same property in base type 'Result<never, E>'.
  Type '() => boolean' is not assignable to type '() => this is OkImpl<never>'.
    Signature '(): boolean' must be a type predicate.

202   isOk(): boolean {
      ~~~~

src/Utils/Result.ts:206:3 - error TS2416: Property 'isErr' in type 'ErrImpl<E>' is not assignable to the same property in base type 'Result<never, E>'.
  Type '() => boolean' is not assignable to type '() => this is ErrImpl<E>'.
    Signature '(): boolean' must be a type predicate.

206   isErr(): boolean {

Found 38 errors in 7 files.

Errors  Files
     2  src/HIR/BuildHIR.ts:196
     2  src/HIR/Environment.ts:745
     2  src/ReactiveScopes/CodegenReactiveFunction.ts:290
    14  src/Utils/Result.ts:96
     2  src/Validation/ValidateNoRefAccesInRender.ts:201
     2  src/Validation/ValidateNoSetStateInRender.ts:115
    14  src/__tests__/Result-test.ts:13

```sh
tsc
tsc --version
Version 5.5.3
```
Copy link

vercel bot commented Jul 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2024 11:02am

@poteto
Copy link
Member

poteto commented Jul 16, 2024

Thanks!

@poteto poteto merged commit 6c0386e into facebook:main Jul 16, 2024
36 checks passed
@nikeee nikeee deleted the fix-result-combinator-returns branch July 17, 2024 00:04
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
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.

3 participants