Improve TypeScript inference for common action pattern #3878
Conversation
|
| Name | Type |
|---|---|
| create-remix | Patch |
| remix | Patch |
| @remix-run/architect | Patch |
| @remix-run/cloudflare | Patch |
| @remix-run/cloudflare-pages | Patch |
| @remix-run/cloudflare-workers | Patch |
| @remix-run/deno | Patch |
| @remix-run/dev | Patch |
| @remix-run/eslint-config | Patch |
| @remix-run/express | Patch |
| @remix-run/netlify | Patch |
| @remix-run/node | Patch |
| @remix-run/react | Patch |
| @remix-run/serve | Patch |
| @remix-run/server-runtime | Patch |
| @remix-run/vercel | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
kentcdodds
changed the title
| if (actionData?.errors?.email) { | ||
| // focus email | ||
| } else if (actionData?.errors?.password) { | ||
| // focus password | ||
| } |
There was a problem hiding this comment.
Note: this test technically runs fine, but fails TypeScript on these two if statements.
|
Here's this test in a TypeScript playground And here it is with what I think is everything we need to know how to change it to fix things for this case: |
|
would this approach work for you? I think TS is complaining because the type is either you can also get around this issue by just accessing the fields in a type-unsafe way: |
|
Thanks @just-toby, but I'd rather not use that because what we're going for is inference. If I change "password" to "secret" in my code, I also have to update the type which is undesirable. |
|
I think the bug is that |
|
What if you explicitly declare return type of the function such as: |
React.useEffect(() => {
if (actionData?.errors) {
if ('email' in actionData.errors) {
// focus email
} else if (actionData?.errors?.password) {
// focus password
}
}
}, [actionData]);satisfies the type checker. Does this work for your case? |
|
Thanks @kulshekhar, that does indeed make TypeScript happy about that code, but it's a pain to do that in JSX. Try this example. @sinansonmez, thanks, but your suggestion is the same as @just-toby's which I explained won't work for what we're trying to do with inference. |
|
Would something like this satisfy requirements? const hasEmailError = (actionData: unknown): actionData is { errors: { email: string } } => {
return false
}
const hasPasswordError = (actionData: unknown): actionData is { errors: { password: string } } => {
return false
}
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function Component() {
let actionData = useActionData<typeof action>();
const email = hasEmailError(actionData) ? actionData.errors.email : null;
const password = hasPasswordError(actionData) ? actionData.errors.password : null;
React.useEffect(() => {
if (email) {
// focus email
} else if (password) {
// focus password
}
}, [actionData]);
return (
<div>
{email && (
<div className="pt-1 text-red-700" id="email-error">
{email}
</div>
)}
</div>
);
}If so,
|
|
Potential fix, replace the definition of // Turn `{ a: someType | undefined }` into `{ a?: undefined }`
type UndefinedOptionals<T> = Merge<
{
// For properties that aren't unions including `undefined`, keep them as is
[k in keyof T as undefined extends T[k] ? never : k]: T[k];
},
{
// For properties that _are_ unions including `undefined`, make them optional (`?`) and remove `undefined` from the union
[k in keyof T as undefined extends T[k] ? k : never]?: Exclude<T[k], undefined>;
}
>;PR is here: #3879 |
|
Hey @kentcdodds I don't think it is possible to conform TS with the original approach. // if (typeof email !== "string" || !email) {
return json({ errors: { email: "Email is required" } });
// }
// if (typeof password !== "string" || !password) {
return json({ errors: { password: "Password is required" } });
// }The shape changes in the two return statements so it is resolving to a union: A way to satisfy TS without going crazy would be to return empty strings: // if (typeof email !== "string" || !email) {
return json({ errors: { email: "Email is required", password: '' } });
// }
// if (typeof password !== "string" || !password) {
return json({ errors: { password: "Password is required", email: '' } });
// }This way the type of the action will be: |
|
maybe the problem here is that union types, when used in generic conditional statements, become distributive? so if |
|
Also, in my personal projects I've retyped export type UseDataFunctionReturn<T extends DataOrFunction> = T extends (
...args: any[]
) => infer Output
? Awaited<Output> extends TypedResponse<infer U>
? U
: Awaited<ReturnType<T>>
: Awaited<T>;
// ...
// if (typeof email !== "string" || !email) {
return json({ errors: { email: "Email is required", password: undefined } });
// }
// if (typeof password !== "string" || !password) {
return json({ errors: { password: "Password is required", email: null } });
// } |
|
Thanks everyone for the help! I'm beginning to think that what it's doing is more correct than what I want it to do. I'm going to play around with it a bit still. Will let you all know what I decide to recommend here. |
Implementing a |
|
If you're open to recommending alternate approaches, in this scenario, I use async function action({ request }: ActionArgs) {
let formData = await request.formData()
let email = formData.get("email")
let password = formData.get("password")
if (typeof email !== "string" || !email) {
return json({
error: "emailRequired" as const,
message: "Email is required",
})
}
if (typeof password !== "string" || !password) {
return json({
error: "passwordRequired" as const,
message: "Password is required",
})
}
return redirect("/hooray")
}
function Component() {
let actionData = useActionData<typeof action>()
React.useEffect(() => {
if (actionData?.error === "emailRequired") {
// focus email
} else if (actionData?.error === "passwordRequired") {
// focus password
}
}, [actionData])
return (
<div>
{actionData?.error === "emailRequired" && (
<div className="pt-1 text-red-700" id="email-error">
{actionData.message}
</div>
)}
{actionData?.error === "passwordRequired" && (
<div className="pt-1 text-red-700" id="password-error">
{actionData.message}
</div>
)}
</div>
)
}This exact approach might not look that nice, but hopefully it gives some ideas! |
|
For reference, I believe this is essentially the same conversation as these two threads regarding undefined fields in loader and action inferred types: (There’s a simpler and more serious bug here, which is that properties which should have type |
|
TLDR: Here's the solution https://tsplay.dev/w1PdlW First, this is a typescript "problem", the following won't compile. let x = {} as { a?: string } | { b?: string }
type Test = keyof typeof x
// ^? never
if (x.a) {
// ~ [1]
console.log(x.a.slice())
// ~ [1]
}
// [1]:
// Property 'a' does not exist on type '{ a?: string | undefined; } | { b?: string | undefined; }'.
// Property 'a' does not exist on type '{ b?: string | undefined; }'.(2339)But it's not a "problem" because why should you be allowed to access property
That works because the return type of let x = {} as { a?: string, b?: undefined } | { b?: string, a?: undefined }
type Test = keyof typeof x
// ^? "a" | "b"
if (x.a) { // compiles
console.log(x.a.slice()) // compiles
}Now you can access The correct way to solve this problem is to not solve it but rather not create it in the first place, ie model your data in a way that is coherent with the way types work, which is could be like this #3878 (comment) for example. But if for whatever reason we don't want to do that, then how can we solve the problem? Here are a few ways...
declare const createX: () => { a?: string } | { b?: string }
let x = createX()
if ("a" in x && x.a) {
console.log(x.a.slice())
}
import { p, pa } from "@sthir/predicate/macro"
declare const createX: () => { a?: string } | { b?: string }
let x = createX()
if (pa(x, p(".a")) {
console.log(x.a.slice())
}
declare const createX: () => { a?: string } | { b?: string }
let x = createX()
let y = x as { a?: string, b?: undefined } | { b?: string, a?: undefined }
if (y.a) {
console.log(y.a.slice())
}
declare const createX: () => { a?: string } | { b?: string }
let x = createX()
let y = x as NormalizeKeys<typeof x>
if (y.a) {
console.log(y.a.slice())
}
type NormalizeKeys<T, TCopy = T> =
T extends unknown
? T extends object
? & T
& { [_ in
Exclude<TCopy extends unknown ? keyof TCopy : never, keyof T>
]?: never
}
: T
: never
declare const createX: () => NormalizeKeys<{ a?: string } | { b?: string }>
type NormalizeKeys<T, TCopy = T> =
T extends unknown
? T extends object
? & T
& { [_ in
Exclude<TCopy extends unknown ? keyof TCopy : never, keyof T>
]?: never
}
: T
: never
let x = createX()
if (x.a) {
console.log(x.a.slice())
}So in our case because we ourselves produce the type, we can go with the option 5. Although our case is a bit different, we want to transform this type... type X =
| { foo: string, bar: { a?: string } }
| { baz: string, bar: { b?: string } }
| { hello: string } to... type XNormalized =
| { foo: string, bar: { a?: string, b?: never } }
| { baz: string, bar: { b?: string, a?: never } }
| { hello: string }So we don't want to touch the root object, but rather normalize the object at type X =
| { foo: string, bar: { a?: string } }
| { baz: string, bar: { b?: string } }
| { hello: string }
type XNormalized =
NormalizeBarKeys<X>
type NormalizeBarKeys<T, TCopy = T> =
T extends { bar: infer B }
? & T
& { bar:
{ [_ in (
Exclude<
TCopy extends { bar: infer B }
? B extends unknown ? keyof B : never
: never,
keyof B
>
)]?: never
}
}
: TAnd in our case export type UseDataFunctionReturn<T extends DataOrFunction> =
+ NormalizeErrorKeys<
T extends (...args: any[]) => infer Output
? Awaited<Output> extends TypedResponse<infer U>
? SerializeType<U>
: SerializeType<Awaited<ReturnType<T>>>
: SerializeType<Awaited<T>>
+ >;
+ type NormalizeErrorsKeys<T, TCopy = T> =
+ T extends { errors: infer E }
+ ? & T
+ & { errors:
+ { [_ in (
+ Exclude<
+ TCopy extends { errors: infer E }
+ ? E extends unknown ? keyof E : never
+ : never,
+ keyof E
+ >
+ )]?: never
+ }
+ }
+ : TAnd here's the final code (same as TLDR)... https://tsplay.dev/w1PdlW But remember type X =
| { errors: { type: "foo", foo: string }
| { errors: { type: "bar", bar: string }(although this would be rare because no one would call it "errors" then but "error" instead) This will turn into... type XNormalized =
| { errors: { type: "foo", foo: string, bar?: never }
| { errors: { type: "bar", bar: string, foo?: never }Which the user might not expect in the tooltip. Ofc normalization doesn't change the type mathematically as such, And ofc this is typescript you can do anything you want, so we can write a heuristic that checks if normalization is not required. An example heuristic can be "Don't normalize if you find a key type NormalizeErrorsKeys<T, TCopy = T> =
+ T extends { errors: { type: unknown } } ? T :
T extends { errors: infer E }
? & T
& { errors:
{ [_ in (
Exclude<
TCopy extends { errors: infer E }
? E extends unknown ? keyof E : never
: never,
keyof E
>
)]?: never
}
}
: TOr conversely you could come up with a heuristic that checks if the normalisation is required. I'm not sure what patterns of modeling are popular/recommended, but the point being we can tailor stuff for our needs. What is the right solution to solve this problem depends what is "right" for us. The correct solution is to not use the error pattern in the OP. But how much correctness is right? The solution that caters to DX for people who use such error pattern is normalization. But how much catering to DX is right? The solution that has the most accessible/easy code (in the remix codebase) is the one without normalization. But how much having accessible/easy code is right? The solution that requires least typescript experts for maintaince is without normalization. But how much not requiring typescript experts is right? What is right? Good luck ;) Also feel free to ask how those types work (I didn't write because it could be lengthy and maybe there are people here who do understand them without an explanation), or any other questions. |
|
@kentcdodds Sorry, just seeing this. I think the This can be fixed with some postprocessing inside type TestType =
| {errors: {a: string}}
| {errors: {b: string; c: string}}
| never; // redirect -> TypedResponse<never>
type Processed = PostprocessActionReturnType<TestType>;
/*
{
errors: {
a?: string | undefined;
b?: string | undefined;
c?: string | undefined;
};
}
*/Worth noting that this "postprocessing" step would happen inside As implemented, there is always an type TestType = {data: string}
type Processed = PostprocessActionReturnType<TestType>;
/*
{
data: string;
errors: undefined;
}
*/As some others have noted though, you probably don't want to short-circuit the error reporting every time a new error is discovered. Better to surface as many errors as possible to the user at once—may be worth recommending a pattern like this in official docs: export const action: ActionFunction = async ({ request }) => {
const formData = await request.formData();
const errors: Partial<Record<"email" | "password">, string>;
const email = formData.get("email");
const password = formData.get("password");
if (!validateEmail(email)) {
errors.email = "Invalid email";
}
if (typeof password !== "string") {
errors.password = "Invalid password";
}
return json({ errors }, { status: 400 });
} |
|
I talked with @kentcdodds , and the original issue is solved by #3879 . As for normalizing the types: It seems that Typescript only normalizes the types when the function directly returns plain objects, but not when returning the result of other functions that return plain objects. See the TS playground example in my tweet and compare the types inferred for I don't think we want to diverge too far from what TS normally does, so going to hold off on forced normalizing. |
|
@kentcdodds feel free to reopen and merge the tests you added if you think that'd be valuable. |
If you're interested in helping out, the best thing to do is rework this TypeScript Playground in a way that allows type inference (rather than explicit typing) and doesn't require weird stuff in the JSX.
I'm trying to figure out the best way to handle this situation. This sort of thing works fine with regular TS, so there's something in our serialized type that's not allowing this pattern. Help is appreciated!
The error I'm getting is:
And