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

docs(tutorials/blog): add TS 4.9's satisfies in combination with LoaderFunction #4675

Closed
wants to merge 10 commits into from

Conversation

tom-sherman
Copy link

Closes: #

  • Docs
  • Tests

Testing Strategy:

I don't think this is needed as it's just a utility type. The interesting part happens in userland when we use the satisfies keyword.


Right now because of the way you need to add types for loaders you can get into a situation where the return type of useLoaderData is never:

const loader = () => "not a response";
const data = useLoaderData<typeof loader>();
//    ^? never

ideally we'd like typescript to error when we return the string instead of a Response. We can use the new satisfies keyword for this:

// ERROR: Type 'number' is not assignable to type 'Response'
const loader = (() => "not a response") satisfies LoaderFunction;

The loader now has a type error but it still allows for the inference of the return type to work with useLoaderData.

@changeset-bot
Copy link

changeset-bot bot commented Nov 23, 2022

⚠️ No Changeset found

Latest commit: d466256

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Nov 23, 2022

Hi @tom-sherman,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Nov 23, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@tom-sherman tom-sherman changed the base branch from main to dev November 23, 2022 11:09
@tom-sherman
Copy link
Author

Ok so the Typescript change can be reverted, I got confused because the version of remix I have doesn't have a LoaderFunction type.

@brophdawg11 do you think the docs change is valuable at all?

@brophdawg11
Copy link
Contributor

yeah I think pointing out how satisfies can enhance the loader/action type safety is nice! I'm going to defer to other folks though on where the best spot for that would be. I'm also curious if we need to consider updating our create-remix templates to use TS 4.9+ if we're going to start recommending those features, or if we can just be clear in our docs that it requires you be on a new enough version of TS.

@sergiodxa
Copy link
Member

Question, can you use satisfies without arrow functions?

@brophdawg11
Copy link
Contributor

@tom-sherman actually, since you can technically return anything (type AppData = any) - does satisfies even help in that case? It might be something where it only helps if you define a more narrowly-typed loader function on your own?

@tom-sherman
Copy link
Author

@sergiodxa Unfortunately not, it appears the syntax doesn't support it right now - you can only use satisfies with an expression.

@brophdawg11 Oh you're absolutely right 🤦 in which case I don't think this even deserves a place in the docs. It might make sense as a more editorialised blog post I guess (I'm already writing one on my own blog)

@tom-sherman
Copy link
Author

@sergiodxa A workaround is something like this:

type Fn = () => number;

function foo() { return "ds" }

foo satisfies Fn; // type error

Not recommended tho because compilers will likely output a foo; statement.

@pcattori
Copy link
Contributor

@sergiodxa A workaround is something like this:

type Fn = () => number;

function foo() { return "ds" }

foo satisfies Fn; // type error

Not recommended tho because compilers will likely output a foo; statement.

One function-compatible option is:

(function hello() {
    return "world"
}) satisfies () => number;

@pcattori
Copy link
Contributor

We're already working on upgrading esbuild to 0.15.13 for satisfies support. You can check #4639 for progress on that.

@pcattori
Copy link
Contributor

pcattori commented Nov 23, 2022

@tom-sherman actually, since you can technically return anything (type AppData = any) - does satisfies even help in that case? It might be something where it only helps if you define a more narrowly-typed loader function on your own?

There's still value in using satisfies even then since it will make sure the function params are correct for example.

@tom-sherman
Copy link
Author

One function-compatible option is:

(function hello() {
    return "world"
}) satisfies () => number;

@pcattori that's a function expression/anonymous function though. You can't do this for example

export (function hello() {
    return "world"
}) satisfies () => number;

@pcattori
Copy link
Contributor

@pcattori that's a function expression/anonymous function though. You can't do this for example

export (function hello() {
    return "world"
}) satisfies () => number;

Yea, if you want (1) export on the same line as the definition and (2) to use satisfies, then I don't think there's a way to do it with a named function.

The only way would be:

export const hello = (function () {
    return "world"
}) satisfies () => number;

but the main benefit of named functions is that they can be referenced before their definition, which you would lose out on with this^.

Personally, I prefer to use arrow functions, but yes its unfortunate for those who use function that satisfies is not supported better for it AFAIK.

@tom-sherman
Copy link
Author

Btw, shameless plug 😅 https://tom-sherman.com/blog/remix-typescript-satisfies

@pcattori
Copy link
Contributor

pcattori commented Dec 9, 2022

https://tom-sherman.com/blog/remix-typescript-satisfies

FYI we export a LoaderFunction type from @remix-run/node (and the other runtime packages too), so you can use that instead of defining Loader yourself: satisfies LoaderFunction

I guess you already knew that since this PR uses LoaderFunction in the docs 😅

@pcattori
Copy link
Contributor

pcattori commented Dec 9, 2022

Looks like tests are failing because the markdown plugin for eslint can't parse TS 4.9 satisfies.

Possible solution: eslint/eslint-plugin-markdown#114 (comment)

docs/tutorials/blog.md Outdated Show resolved Hide resolved
docs/tutorials/blog.md Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey changed the title Add LoaderFunction type for usage with satisfies in TypeScript 4.9 docs(tutorials/blog): add TS 4.9's satisfies in combination with LoaderFunction Dec 9, 2022
@tom-sherman
Copy link
Author

tom-sherman commented Dec 10, 2022

I guess you already knew that since this PR uses LoaderFunction in the docs 😅

@pcattori Weird one, I wasn't actually aware of that type when I wrote the blog because I'm on an experimental channel (deffered) that doesn't have that type 😆

docs/tutorials/blog.md Outdated Show resolved Hide resolved
@moishinetzer
Copy link

Is there any updated best practice/solution to solve this?

I want to be able to take advantage of useLoaderData<typeof loader>() AND make sure that my loader functions return the correct types.

@tom-sherman
Copy link
Author

tom-sherman commented Feb 6, 2023

@moishinetzer The simplest way to do that would be to add a return type annotation to the loader eg.

export async function loader({ context }: LoaderArgs): Promise<TypedResponse<{ posts: Post[], tags: string[] }>> {
  const blog = new BlogData(context);

  const [posts, tags] = await Promise.all([
    blog.listAllPosts(),
    blog.listAllTags(),
  ]);

  return json({
    posts,
    tags,
  });
}

const { posts, tags } = useLoaderData<typeof loader>();

You can use satisfies if you wanted to assert a partial data shape at the loader level and have the concrete return type inferred. I touch on this in my blog post.

Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

@tom-sherman Could you rebase this to target main?
Documentation & templates updates go directly to main

@pcattori pcattori self-assigned this Aug 8, 2023
@@ -195,6 +195,34 @@ export default function Posts() {

Hey, that's pretty cool. We get a pretty solid degree of type safety even over a network request because it's all defined in the same file. Unless the network blows up while Remix fetches the data, you've got type safety in this component and its API (remember, the component is already its own API route).

We can take the type safety a step further in TypeScript 4.9.0 using the `satisfies` keyword. This will ensure that we always return a type of `Response` from the loader instead of accidentally returning another type which would result in a runtime error.

```ts filename=app/routes/posts/index.tsx lines=[1,23]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```ts filename=app/routes/posts/index.tsx lines=[1,23]
```ts filename=app/routes/posts/index.tsx

Comment on lines +201 to +202
import type { LoaderFunction } from "@remix-run/node";
import { json } from "@remix-run/node";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import type { LoaderFunction } from "@remix-run/node";
import { json } from "@remix-run/node";
import type { LoaderFunction } from "@remix-run/node"; // or cloudflare/deno
import { json } from "@remix-run/node"; // or cloudflare/deno

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Aug 8, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Aug 10, 2023
@pcattori
Copy link
Contributor

Looks like the upstream issues with eslint plugins and TS 4.9 have been resolved as yarn lint passes locally for me with the suggested edits.

That said, we are no longer using this tutorial in the v2 docs, so closing this PR.

@pcattori pcattori closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants