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

Contextual type can't be provided to a mapped type intersected with an object type #48812

Open
Andarist opened this issue Apr 22, 2022 · 16 comments Β· Fixed by #48668 Β· May be fixed by #52095
Open

Contextual type can't be provided to a mapped type intersected with an object type #48812

Andarist opened this issue Apr 22, 2022 · 16 comments Β· Fixed by #48668 Β· May be fixed by #52095
Labels
Bug A bug in TypeScript
Milestone

Comments

@Andarist
Copy link
Contributor

Bug Report

πŸ”Ž Search Terms

intersection, mapped type, contextual type

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ

⏯ Playground Link

Playground

πŸ’» Code

type Action<TEvent extends { type: string }> = (ev: TEvent) => void;

interface MachineConfig<TEvent extends { type: string }> {
  schema: {
    events: TEvent;
  };
  on?: {
    [K in TEvent["type"]]?: Action<TEvent extends { type: K } ? TEvent : never>;
  } & {
    "*"?: Action<TEvent>;
  };
}

declare function createMachine<TEvent extends { type: string }>(
  config: MachineConfig<TEvent>
): void;

createMachine({
  schema: {
    events: {} as { type: "FOO" } | { type: "BAR" },
  },
  on: {
    FOO: (ev) => {
      ev.type; // should be 'FOO', but `ev` is typed implicitly as `any`
    },
  },
});

πŸ™ Actual behavior

An implicit any pop-ups when the contextual type could be, somewhat easily, provided.

πŸ™‚ Expected behavior

This should just work 😜 I know a workaround for this issue - the workaround is to use a single mapped type instead of an intersection and just "dispatch" to the correct value in the template~ part of the mapped type, like here. However, this is way less ergonomic than an intersection AND the mapped type is no longer homomorphic which could matter for some cases (well, the original mapped type here is not homomorphic either, but it could be)

I already have a draft PR open to fix this issue, here. I only need some help with the stuff mentioned in the comment here

@Andarist
Copy link
Contributor Author

@RyanCavanaugh @andrewbranch let me know if I can help anyhow with resolving the issue from #49307 . I wonder though if the issue described there isn't on the verge of abusing the internals. I don't intend to throw any stones here cause I'm far away from the TS "comfort zone". At times though, I wouldn't be that surprised if my code would break because of some changes in the TS internals - it's just on the bleeding edge so I'm aware that I can get hurt 🀣

I think that reverting the change was a good call since you are just before a new stable release and RTK is quite a popular library. However, I wonder how compelling the code presented there is. It's not exactly that it was relying on well-documented behavior.

While we could try to improve the code for that other case - I guess that figuring out heuristics for that is way outside of my league and might be quite tricky even for your team. IIRC I was testing things out with a couple of scenarios involving indexers and concrete properties and I think that usually the concrete properties just "won". So given that, I'm not sure if an easily understandable heuristic can be made to make it work. So I really wonder if the best solution here is to give RTK the time to release new library versions that would be compatible with the new heuristics (from the reverted PR).

If I understand correctly, the types from RTK try to provide types using an unusual technique. They try to iterate through properties of the inferred type and validate it, so they kinda introduce another compiler pass at the computed type. It seems that there is an appetite for such things from the library authors. I can see some use cases for "validating" inferred types, and even for improved control over displayed errors (throw types). Maybe it's an opportunity to make such things first-class citizens of TypeScript? People already do this kind of stuff in a couple of ways and it would be great to have a clear way of doing this.

cc @phryneas @markerikson

@markerikson
Copy link

@Andarist fwiw, Lenz tried to come up with an alternate implementation, but couldn't find anything that would work with both 4.7 and 4.8. That led to him trying to hack together a package that provides types to figure out what TS version we're dealing with just to be able to figure out what type to use in that case:

reduxjs/redux-toolkit#2547

that's really not a good fix long-term, though.

I didn't write this part of the types myself, and I don't actually understand what's going on with this issue or the PR you filed to fix it, so I don't have more feedback here beyond "our code worked, and then this TS PR broke it" :)

@phryneas
Copy link

I can see some use cases for "validating" inferred types, and even for improved control over displayed errors (#40468). Maybe it's an opportunity to make such things first-class citizens of TypeScript? People already do this kind of stuff in a couple of ways and it would be great to have a clear way of doing this.

That would be absolutely amazing.

There have been multiple ways of doing things like this and they all are kinda flaky and break left and right when just changing the smallest details. (For example this hack by StackOverflow user jcalz).

We would absolutely love to be able to move away from those hacks to a more "official" way of second-pass validation.

@markerikson
Copy link

(I don't even know what "second-pass validation" even means :) )

@phryneas
Copy link

Lemme try an explanation :D

We're letting the user provide an object in the form

{
  reducer(state, action: PayloadAction<something>) {},
  prepare(arg) { return actionPayload } 
}

now, we want the return type of actionPayload to be the same as something up in PayloadAction. But we cannot just add a generic <T> somewhere to make sure that's the case - because this object is not the only object being passed in, but one of many objects inside a config object:

{
  reducers: {
    foo: { reducer: ... ,  prepare: ... },
    bar: { reducer: ... ,  prepare: ... },
  }
}

Now, TS has no syntax to allow for different ActionPayload types for foo and bar while having those internally consistent (foo only has FooActionPayload both on reducer and prepare and bar only has BarActionPayload both on reducer and prepare).

So what we do is that we let TypeScript infer this whole configuration object including all reducers (the "first pass") and then, when we have that, we use that ConfigObject to restrict it against itself (the "second pass") - ConfigObject extends Validated<ConfigObject> where Validated is a generic that infers FooActionPayload from ReturnType<ConfigObject['reducers']['foo']['prepare']> and makes sure that the second argument to reducer matches that type.

It's amazing that we could do something like that in the first place, but it's also pretty necessary here to make the api work in a type-safe manner.

@RyanCavanaugh
Copy link
Member

Something that would be a huge help here is some fully-concrete examples of, like, five calls that should work and five calls that should fail. As much as we try to understand most popular libraries, we're not Redux experts, but given the problem of "Here's some behavior we can't represent", we have a much better chance of some form of success (either in doing typing tricks you might not be aware of, or adding higher-level primitives that address a broader class of use cases).

@phryneas
Copy link

Good point, here is a test case:
Playground link

import { createSlice, PayloadAction } from '@reduxjs/toolkit'

{
  createSlice({
    name: "test",
    initialState: 0,
    reducers: {
      // normal reducer
      test(state, action: PayloadAction<number>) {
        return state + action.payload
      }
    }
  })
}


{
  createSlice({
    name: "test",
    initialState: 0,
    reducers: {
      // reducer with prepare
      test: {
        reducer(state, action: PayloadAction<number>) {
          return state + action.payload
        },
        prepare(arg: number) {
          return { payload: arg * 2 }
        }
      }
    }
  })
}


{
  createSlice({
    name: "test",
    initialState: 0,
    reducers: {
      // reducer with incorrect prepare
      test: {
        // @ts-expect-error action payload needs to be a number, as returned by `prepare`
        reducer(state, action: PayloadAction<string>) {
          return state + action.payload
        },
        prepare(arg: number) {
          return { payload: arg * 2 }
        }
      }
    }
  })
}


{
  createSlice({
    name: "test",
    initialState: 0,
    reducers: {
      // normal reducer
      test1(state, action: PayloadAction<number>) {
        return state + action.payload
      },
      // reducer with prepare
      test2: {
        reducer(state, action: PayloadAction<number>) {
          return state + action.payload
        },
        prepare(arg: number) {
          return { payload: arg * 2 }
        }
      },
      // reducer with incorrect prepare
      test3: {
        // @ts-expect-error action payload needs to be a number, as returned by `prepare`
        reducer(state, action: PayloadAction<string>) {
          return state + action.payload
        },
        prepare(arg: number) {
          return { payload: arg * 2 }
        }
      },
      // another reducer with incorrect prepare and different payload type
      test4: {
        // @ts-expect-error action payload needs to be { value: number }, as returned by `prepare`
        reducer(state, action: PayloadAction<{value: string}>) {
          return state + action.payload.value
        },
        prepare(arg: number) {
          return { payload: { value: arg * 2 }}
        }
      }
    }
  })
}

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 16, 2022

@phryneas let me check my understanding -- am I correct that this call should be legal from redux's perspective (I see that TS rejects it) ?

  createSlice({
    name: "foo",
    initialState: 0,
    reducers: {
      alpha: {
        reducer(state, action: PayloadAction<number>) {
          return action.payload;
        },
        prepare() {
          return { payload: 0 };
        }
      },
      beta: {
        reducer(state, action: PayloadAction<string>) {
          return action.payload;
        },
        prepare() {
          return { payload: "" };
        }
      }
    }
  });

@markerikson
Copy link

Looking at it, no, that should fail.

In both reducers, the type of {payload} in prepare() lines up with the type of action: PayloadAction<x> in reducer. That part's good.

But the initialState is a number. alpha.reducer returns a number, so that's fine. But beta.reducer returns a string, so that should error because it doesn't match the state type.

@Andarist
Copy link
Contributor Author

Could we revisit this PR and the original fix? How big of a problem would it be for RTK to adjust its typedefs? Can typesVersions help here aka can we even write a definition that would work with the mentioned fix? Based on the executed compatibility tests of the fix the RTK was the only package affected by this.

@phryneas
Copy link

@Andarist afaik there is no way to write a solution that works both in old TS & new TS versions. I would love to avoid introducting typesVersions just for this since they are a horror to maintain. If #50196 would land to some extent, we could easily use that.

@Andarist
Copy link
Contributor Author

Do you know how to write a version compatible with this fix? I agree that typesVersions are hard to maintain if you author those types separately but perhaps we could figure out a script that would output multiple variants from a single source file (assuming that #50196 doesn't get implemented soon enough/at all)?

@phryneas
Copy link

@Andarist in this case, replacing
{} with

import('@phryneas/ts-version').TSVersion.AtLeast<4, 8> extends true
      ? ACR[T]
      : {}

See
https://github.com/reduxjs/redux-toolkit/pull/2547/files#diff-ee57feab8b392faa360ea66691a847a2dbed2f82486f5726c3723d6a194f22f0R230-R240

But this is at this point a hack on the shoulders of a hack, with a hacky hat. Not very happy actually doing this.

@Andarist
Copy link
Contributor Author

It definitely ain't pretty but I've seen worse 🀣 I remembered that you have created this package - but I was also thinking about a script that would "resolve" such conditions and output multiple different typesVersions based on a single source file. Would that be less of a hack?

@phryneas
Copy link

I was thinking of some type of "templating engine for typesVersions" - maybe based on annotations in the source file or something, but I never got around to write it. If you need a new project I'd be glad to use it ^^

@Andarist
Copy link
Contributor Author

Andarist commented Sep 20, 2022

I see what you did there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
5 participants