-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Comments
@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. |
@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: 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" :) |
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. |
(I don't even know what "second-pass validation" even means :) ) |
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 {
reducers: {
foo: { reducer: ... , prepare: ... },
bar: { reducer: ... , prepare: ... },
}
} Now, TS has no syntax to allow for different ActionPayload types for 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 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. |
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). |
Good point, here is a test case: 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 }}
}
}
}
})
} |
@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: "" };
}
}
}
}); |
Looking at it, no, that should fail. In both reducers, the type of But the |
Could we revisit this PR and the original fix? How big of a problem would it be for RTK to adjust its typedefs? Can |
Do you know how to write a version compatible with this fix? I agree that |
@Andarist in this case, replacing
But this is at this point a hack on the shoulders of a hack, with a hacky hat. Not very happy actually doing this. |
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 |
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 ^^ |
Bug Report
π Search Terms
intersection, mapped type, contextual type
π Version & Regression Information
β― Playground Link
Playground
π» Code
π 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
The text was updated successfully, but these errors were encountered: