-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Single-fetch typesafety: defineLoader
and defineAction
#9372
Conversation
🦋 Changeset detectedLatest commit: cdbea62 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Do we want to remove |
Also - if you don't use
I don't know if I was expecting a export function loader({ request, response }: LoaderFunctionArgs) {
return {
string: "hello",
date: new Date(),
};
}
export default function Index() {
const loaderData = useLoaderData<typeof loader>();
console.log(loaderData.date.toISOString());
// ^ Should this be typed as a date without the use of defineLoader?
return (...);
} |
Single-fetch supports the following return types: | ||
|
||
```ts | ||
type Serializable = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this type exported somewhere?
| number | ||
| bigint | ||
| Date | ||
| URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could URLSearchParams be added to this too? I remember someone reporting in Discord that they tried to return this and it failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using Jacob's turbo-stream library under-the-hood so support for URLSearchParams
would need to be added there.
}); | ||
|
||
export const action = defineAction(() => { | ||
return { hello: "world", badData: new CustomType() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if CustomType class has a toJSON? Couldn't the badData.toJSON()
method be called and then return as a plain JSON? So you will not get a CustomType client-side but at least the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract for single-fetch (and turbo-stream
which is what it is using under-the-hood) is that a type T
in becomes a type T
out. So CustomType
-> string
is not supported. If you want you can manually call toJSON()
:
export let loader = () => {
return { a: myCustomTypeInstance.toJSON() }
}
The point is to have less coercion and magic. Having the serialization be basically a glorified ID function is the thing that unlocks a bunch of the simplified types features. Plus it means we can support recursive types much better than the current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergiodxa FIY, I raised the same question on Discord: https://discord.com/channels/770287896669978684/1233404747885576293/1233417203693523104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross posting my response from discord as well for visibility
We're hesitant to add to much "custom" stuff like this to turbo-stream because it's a stopgap solution that will be replaced with the RSC wire streaming format once we land the RSC work. So the limitations of what can and can't be streamed are going to eventually be defined by React, not us
I.e., if we add toJson
support now, folks will start relying on it. Then if we incorporate RSC and it doens't support toJSON
, it would be a breaking change for your apps. So we're keeping this simpler now until we know in greater detail how the RSC story shakes out.
@brophdawg11 looks like there are still a couple places in the codebase that rely on |
See changeset for more detail