-
Notifications
You must be signed in to change notification settings - Fork 2.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
Incorrect type from useLoaderData
#3931
Comments
useLoader
useLoaderData
Maybe this is related to TypeScript, but should I use the Thought we should completely replace it |
I confirm I have the same "bug" of TS types with the tutorial |
Another example: import { useLoaderData } from "@remix-run/react";
import { json } from "@remix-run/server-runtime";
export function loader() {
const x = Math.random();
if (x === 1) {
return json({ a: "stringA" });
}
if (x === 2) {
return json({ a: 2});
}
if (x === 3) {
return json({ a: 2, b: 2 });
}
return json({ c: "stringC" });
}
export default function AppTest() {
const loaderData = useLoaderData<typeof loader>();
...
} const loaderData: SerializeObject<{
a: string;
}> | SerializeObject<{
a: number;
}> | SerializeObject<{
c: string;
}>
Changing the type of export function loader() {
const x = Math.random();
if (x === 1) {
return json({ a: "stringA" });
}
if (x === 2) {
return json({ a: 2});
}
if (x === 3) {
return json({ a: [2], b: 2 });
}
return json({ c: "stringC" });
}
export default function AppTest() {
const loaderData = useLoaderData<typeof loader>();
...
}
```ts
const loaderData: SerializeObject<{
a: string;
}> | SerializeObject<{
a: number;
}> | SerializeObject<{
a: number[];
b: number;
}> | SerializeObject<{
c: string;
}> The conclusion I was able to draw thus far is if an object is returned that has a property of the same type, but an extra property added as well, it doesn't get picked up. If I take the first example and completely remove the export function loader() {
const x = Math.random();
if (x === 1) {
return json({ a: "stringA" });
}
// if (x === 2) {
// return json({ a: 2});
// }
if (x === 3) {
return json({ a: 2, b: 2 });
}
return json({ c: "stringC" });
}
export default function AppTest() {
const loaderData = useLoaderData<typeof loader>();
...
} const loaderData: SerializeObject<{
a: string;
}> | SerializeObject<{
a: number;
b: number;
}> | SerializeObject<{
c: string;
}> |
Unfortunately I think we're going to continue to get a lot of these weird edge cases. The issue is that Remix is trying to convert your actual data type, with all its various forms, and convert that to what the type would look like if you replaced any non-JSON types with their serialized versions. They are using a home-grown function to do this, but plan to use I side-stepped this with |
Thank you @kiliman, we started using |
Can this be included in remix, types returned from remix are unusable. When using prisma, @kiliman Thanks! |
Sadly, even with remix-typedjson it still doesn't work for me. export interface LinksGroupProps {
icon: TablerIcon;
label: string;
initiallyOpened?: boolean;
links?: { label: string; link: string }[];
}
export const loader = async ({request}: LoaderArgs) => {
const data = {
main: [
{
link: "/taskapp",
label: "Task App",
links: [
{
link: "/tasks",
label: "Tasks"
},
{
link: "/processes",
label: "Processes"
},
{
link: "/cases",
label: "Cases"
}
]
},
{
link: "/modeler",
label: "Modeler",
},
{
link: "/admin",
label: "Admin"
},
{
link: "/idm",
label: "IDM"
}
],
side: [
{ icon: IconGauge, label: 'Dashboard' },
{
label: 'Market news',
initiallyOpened: true,
links: [
{ label: 'Overview', link: '/' },
{ label: 'Forecasts', link: '/' },
{ label: 'Outlook', link: '/' },
{ label: 'Real time', link: '/' },
],
icon: IconNotes,
},
{
label: 'Releases',
links: [
{ label: 'Upcoming releases', link: '/' },
{ label: 'Previous releases', link: '/' },
{ label: 'Releases schedule', link: '/' },
],
icon: IconCalendarStats,
},
{ label: 'Analytics', icon: IconPresentationAnalytics },
{ label: 'Contracts', icon: IconFileAnalytics },
{ label: 'Settings', icon: IconAdjustments },
{
label: 'Security',
links: [
{ label: 'Enable 2FA', link: '/' },
{ label: 'Change password', link: '/' },
{ label: 'Recovery codes', link: '/' },
],
icon: IconLock,
}
]
}
// return json(data)
return typedjson(data)
}
export default function Tasks() {
//const {main, side} = useLoaderData() as LoaderData
const {main, side} = useTypedLoaderData<typeof loader>()
console.log("side", side)
return (
<>
<HeaderComponent links= {main} />
<NavbarNested links= {side} />
<Outlet/>
</>
);
} It seems like complex type like TablerIcon doesn't get serialized and send correctly across the wire. |
Yes, as explained in You could also try |
Thanks for the clarification. |
For the remix tutorial, adding 'unknown' converstion seemed to make TS happy const { posts } = useLoaderData() as unknown as LoaderData; |
This comment was marked as resolved.
This comment was marked as resolved.
For other people's reference, this is force casting: https://www.w3schools.com/typescript/typescript_casting.php What would be the logical thing to do if done correctly without force casting? Should |
Don't want to sound rude, but a feedback to the Remix Team - |
|
+1 this is definitely confusing and not clear how best to handle. I would fully expect to be able to do this:
given a
quick edit: thinking about this some more, perhaps a |
Yeah, I'm not a big fan of the |
Is there any update on whether a solution akin to |
So a lot has passed since I opened this issue and I read all your comments. The snippet I shared at the start was a simplified version of a For this, take the following export const loader = async ({ request }: LoaderArgs) => {
const url = new URL(request.url);
const shopifyDomain = url.searchParams.get("shop");
const host = url.searchParams.get("host");
if (shopifyDomain && host) {
const session = await shopSession.getSession(request.headers.get("Cookie"));
if (session.get("shopifyDomain") === shopifyDomain) {
const shop = await findShopInAuth({ shopifyDomain });
if (shop) {
const apiKey = Env.get("SHOPIFY_API_KEY");
return {
success: true,
apiKey,
host,
};
}
}
throw redirect(`/api/auth?shop=${shopifyDomain}&host=${host}`);
}
return {
success: false,
};
}; The return type of this Promise<{
success: boolean;
apiKey: string;
host: string;
} | {
success: boolean;
apiKey?: undefined;
host?: undefined;
}>
So you can't correctly discriminate this union: export default function EmbeddedLayout() {
const loaderData = useLoaderData<typeof loader>();
const locale = useLocale();
const { ready } = useTranslation("translation", { useSuspense: false });
if (ready) {
return loaderData.success ? (
<EmbeddedApp {...loaderData}> // error
<Outlet />
</EmbeddedApp>
) : (
<AppProvider i18n={locale} linkComponent={Link}>
<Outlet />
</AppProvider>
);
}
return <></>;
}
type EmbeddedAppProps = React.PropsWithChildren<{
apiKey: string;
host: string;
}>;
declare function EmbeddedApp({ children, apiKey, host }: EmbeddedAppProps); This raise an error since SerializeObject<UndefinedToOptional<{
success: boolean;
apiKey: string;
host: string;
}>> | SerializeObject<UndefinedToOptional<{
success: boolean;
apiKey?: undefined;
host?: undefined;
}>> So, should we use a type LoaderData =
| { success: true; apiKey: string; host: string }
| { success: false };
export default function EmbeddedLayout() {
const loaderData = useLoaderData<LoaderData>();
const locale = useLocale();
const { ready } = useTranslation("translation", { useSuspense: false });
if (ready) {
return loaderData.success ? (
<EmbeddedApp {...loaderData}> // all good
<Outlet />
</EmbeddedApp>
) : (
<AppProvider i18n={locale} linkComponent={Link}>
<Outlet />
</AppProvider>
);
}
return <></>;
} but from my little experience I don't see this approach scalable, since if the I don't like this approach, so instead I started to use return {
success: true,
apiKey,
host,
} as const;
...
return {
success: false,
} as const; From this typescript infer the following type: Promise<{
readonly success: true;
readonly apiKey: string;
readonly host: string;
} | {
readonly success: false;
readonly apiKey?: undefined;
readonly host?: undefined;
}> That is a truly discriminated union, and now SerializeObject<UndefinedToOptional<{
readonly success: true;
readonly apiKey: string;
readonly host: string;
}>> | SerializeObject<UndefinedToOptional<{
readonly success: false;
readonly apiKey?: undefined;
readonly host?: undefined;
}>> This works great, no force casting with A little enhancement from Remix would be to remove from the type keys with @miniplus this could also fix your issue, however I don't know why you never see function loader() {
const x = Math.random();
if (x === 1) {
return { a: "stringA" } as const;
}
if (x === 2) {
return { a: 2 } as const;
}
if (x === 3) {
return { a: 2, b: 2 } as const;
}
return { c: "stringC" } as const;
}
export type Prettify<T> = { [K in keyof T]: T[K] } & {};
type RemoveUndefined<T> = Prettify<{ -readonly [K in keyof T as T[K] extends undefined ? never : K]: T[K]}>
const bar: RemoveUndefined<ReturnType<typeof loader>> = loader(); The type of {
a: "stringA";
} | {
a: 2;
} | {
a: 2;
b: 2;
} | {
c: "stringC";
} Here is a playground @uhrohraggy from the type you posted, |
I have Remix return {
foo: {
bar: "it's a string"
}
} For flat structures it does work also without |
Hey @kwiat1990, could you provide more details for your problem please? |
I think I didn't quite get the last part of your example in which you call |
@kwiat1990 oh sorry, that was a simple typescript example, here is how you should translate it to remix: export const loader = () => {
const x = Math.random();
if (x === 1) {
return { a: "stringA" } as const;
}
if (x === 2) {
return { a: 2 } as const;
}
if (x === 3) {
return { a: 2, b: 2 } as const;
}
return { c: "stringC" } as const;
};
export default function Index() {
const loaderData = useLoaderData<typeof loader>();
if ("a" in loaderData) {
loaderData.a; // "stringA" | 2
}
if ("b" in loaderData) {
loaderData; // { a: 2; b: 2; }
}
if ("c" in loaderData) {
loaderData; // { c: "stringC"; }
}
return <div></div>;
} As I said, the type Prettify<T> = { [K in keyof T]: T[K] } & {};
type RemoveUndefined<T> = Prettify<{
-readonly [K in keyof T as T[K] extends undefined ? never : K]: T[K];
}>;
const loaderData: RemoveUndefined<Awaited<ReturnType<typeof loader>>> =
useLoaderData<typeof loader>(); Or better, write your own function useLoaderData<T extends () => any>(): RemoveUndefined<
Awaited<ReturnType<T>>
> {
return useRemixLoaderData<T>();
} |
OK then, it seems I have made everything correctly but like I said before, with this Typescript throws following error:
One of the things being returned from the loader and what's causing Typescript error is export interface Article {
author: Author;
category: Category;
content: string;
createdAt: string;
id: string;
lead: string;
publishedAt: string;
slug: string;
tags: Tag[];
title: string;
updatedAt: string;
cover?: Image;
readTime?: string;
} |
@kwiat1990 from the error Typescript raised the problem is in the type of A possible solution (although not elegant) could be the following: export default function() {
const loaderData = useLoaderData<typeof loader>();
return <Component
article={
{
...loaderData,
author: {
...loaderData.author,
avatar: {
...loaderData.author.avatar,
createdAt: new Date(loaderData.author.avatar.createdAt)
}
}
}
}/>
} |
In any place or component I expect a I get mentioned error at this place in my code: const loaderData: RemoveUndefined<Awaited<ReturnType<typeof loader>>> =
useLoaderData<typeof loader>(); The bottom line is that with the changes how Remix types loader data a lot of things got broken and currently I don't see any easy way in order to fix it. The |
@kwiat1990 oh sorry, the problem here is that the type from For this, you should also pass |
Thanks for clarification. It feels a bit counterproductive, sort of, to fight against a framework of your choice. I would rather stick to my own interfaces/types for loader data, which I need to write by myself than to use more and more wrappers to get rid of Remix types. |
The issue here is that Remix is trying NOT to lie to you by specifying the actual result type you're getting, which may differ from what you returned from your loader due to JSON conversion. And yes, it can be annoying, especially for nested objects. I'm sure it's difficult to have some automated way to infer this result type for countless return values that will work well in all cases. That's why I punted, and instead of returning a JSON value, I returned my typed JSON, which automatically converts back to its native form. Then I can simply use the actual type from the loader inferred by TypeScript. |
My issue with this behavior is that I need to manage to write some Typescript wrapper for this to work or install yet another dev dependency and hope it works with my custom types. In both cases I'm not the happiest guy in the world. I would really prefer that Remix says: hey, you need type those things on your own. We can't. |
I think the solution here is using e.g. export default function PostList({ posts }: { posts: SerializeFrom<Post[]>; }) {
return (
<div>
Posts!
</div>
);
} |
@graysonhicks sadly this method is not scalable |
For now |
I heard there was a PR to fix this, right? |
Yeah the serialized types have some challenges if you need to maintain the same type across the stack. Basically what @kwiat1990 said, #3931 (comment)
Personally encountered issues where it violates the contract with my Such as here, representing
Anyways, im unenthusiastically working around this by unwrapping the underlying types type UnwrapLoaderData<T extends LoaderFunction> = Awaited<
ReturnType<Awaited<Awaited<ReturnType<T>>['json']>>
>; const data = useLoaderData() as UnwrapLoaderData<typeof loader>() |
I still don't understand why I hear people say network serialization, conversion to string and stuff, but it still doesn't make any sense, this is typescript types, why can't it just return the valid JSON type coming from the data layer? |
@sslgeorge the export const loader = () => {
return {
foo: new Date(),
};
}
export default function App() {
const { foo } = useLoaderData();
return <>{foo.getFullYear()}</>;
} This code is wrong, because the method But isn't This is why the |
Before addressing comments, I want to clarify that Remix's serialization types are meant to emulate Seems simple, but there are tons of little details and nuances to get right. Remix is committed to not mislead you about what the data will actually look like on the client, as mentioned by @nullndr in this comment. My recommendation is to default to use Remix's built-in serialization types(e.g. Additionally, if you know the data types your are using are supported by @kiliman 's remix-typedjson that can also be a great choice, but know that The original issue makes this claim: // expected
{
domain: string;
status: string;
reason?: string;
}
// actual
{
domain: string;
status: string;
reason?: undefined;
} |
{
domain: string;
status: string;
reason: string;
} but in fact, Typescript doesn't agree. Here's a TS playground with no Remix code that demonstrates that TS doesn't think the type for As for the behavior highlighted by @miniplus , this is also Typescript standard behavior, not a Remix issue. See this playground with no Remix code that shows the same behavior. |
As of v0.2.0, https://github.com/kiliman/remix-typedjson#registercustomtype |
Nice! 💪 That still requires a manual step to register, which if you are using 3rd party types might be onerous. I think its a good tradeoff and the right design choice for |
I don't quite understand your post in practical terms @pcattori so was how it was working before wrong? |
export let loader = () => {
// the server will serialize this and send it over the network
// basically, `sendPayload(JSON.stringify(x))`
return json(x)
}
export default Component() {
// now, the client will read the data as json
// basically, `JSON.parse(receivePayload())`
let y = useLoaderData<typeof loader>()
} In 1.19, Remix did a pretty good job of providing types to match |
All reported issues should be fixed by #7605 . You can try it out with the next nightly release (which will get published in a few hours as of the time writing this comment). If any new issues emerge, feel free to open a new GitHub issue for that specific case. |
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
What version of Remix are you using?
1.6.7
Steps to Reproduce
Expected Behavior
the type of
requestedDomains
should beActual Behavior
the type of
requestedDomains
is:See also this discussion where I try some solutions.
The text was updated successfully, but these errors were encountered: