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

Single-fetch typesafety: defineLoader and defineAction #9372

Merged
merged 5 commits into from May 9, 2024
Merged

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented May 3, 2024

// turbo-stream for bare objects
let loader1 = () => ({a: 1})
useLoaderData<typeof loader1>(); // { a: number }

// Jsonify for `json`/`defer`
let loader2 = () => json({a: 1})
useLoaderData<typeof loader2> // JsonifyObject<{a: number}>

// typesafety for args and return type via `defineLoader`
let loader3 = defineLoader(({ request }) => {
  //                          ^? Request
  return {a: 1, b: () => 2}
  //            ^ `b` being a function causes a type error for `defineLoader`
})

See changeset for more detail

Copy link

changeset-bot bot commented May 3, 2024

🦋 Changeset detected

Latest commit: cdbea62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/dev Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
@remix-run/testing Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config Patch

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

@brophdawg11
Copy link
Contributor

Do we want to remove response from LoaderFunctionArgs and ActionFunctionArgs now too and let it come in via defineLoader/defineAction?

@brophdawg11
Copy link
Contributor

Also - if you don't use defineLoader should we still be trying to infer single fetch types? Or are we saying the two are mutually exclusive?

  • LoaderFunctionArgs -> JSON inference
  • defineLoader -> turbo-stream inference

I don't know if I was expecting a Date to be inferred here anymore without opting into defineLoader?

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 =
Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor Author

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() };
Copy link
Member

@sergiodxa sergiodxa May 8, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.

@pcattori
Copy link
Contributor Author

pcattori commented May 8, 2024

@brophdawg11 looks like there are still a couple places in the codebase that rely on response in LoaderFunctionsArgs and ActionFunctionArgs, so left that in for now.

@pcattori
Copy link
Contributor Author

pcattori commented May 8, 2024

Also - if you don't use defineLoader should we still be trying to infer single fetch types? Or are we saying the two are mutually exclusive?

Bare objects are always single-fetch serialized after you enabled single-fetch future flag + types, as intended:

Screenshot 2024-05-08 at 7 52 45 PM Screenshot 2024-05-08 at 7 54 36 PM

@pcattori pcattori merged commit dafe70e into dev May 9, 2024
5 checks passed
@pcattori pcattori deleted the define-loader branch May 9, 2024 00:08
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label May 9, 2024
@brophdawg11 brophdawg11 linked an issue May 9, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants