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

Typesafety improvements #12019

Merged
merged 32 commits into from
Oct 1, 2024
Merged

Typesafety improvements #12019

merged 32 commits into from
Oct 1, 2024

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Sep 19, 2024

See the included decision doc

TODO

  • typegen CLI
  • fix react-router/dev/routes resolution error
  • rename args/props type utils to be different than the typegen'd type names
  • typegen consistent with route files, not just routes.ts config (delete typegen'd file when route no longer exists)
  • clientLoader hydrate API
  • .d.ts instead of .ts for typegen'd files
  • typegen for root route
  • no hardcoded app/ dir
  • decision doc reject solutions: tsplugin, defineLoader + friends
  • changeset

Follow-on work

  • typegen: headers, links, meta, shouldRevalidate, handle, etc.
  • plugin: autocomplete for route exports
  • plugin: jsdoc for route exports
  • plugin: HMR warnings

Copy link

changeset-bot bot commented Sep 19, 2024

🦋 Changeset detected

Latest commit: 7e87c5d

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

This PR includes changesets to release 10 packages
Name Type
@react-router/dev Major
react-router Major
@react-router/fs-routes Major
@react-router/remix-config-routes-adapter Major
@react-router/architect Major
@react-router/cloudflare Major
react-router-dom Major
@react-router/express Major
@react-router/node Major
@react-router/serve Major

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

Copy link
Contributor

@brookslybrand brookslybrand left a comment

Choose a reason for hiding this comment

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

Feedback on "Type inference" decision doc

decisions/0012-type-inference.md Outdated Show resolved Hide resolved
decisions/0012-type-inference.md Outdated Show resolved Hide resolved
> But in the initial SSR render, the `clientLoader` doesn't run, so really you'd need `typeof loader | typeof clientLoader`.
> Unless you also set `clientLoader.hydrate = true` _AND_ provided a `HydrateFallback`. Then `clientLoader` always runs so you'd want just `typeof clientLoader`

That `useLoaderData` generic starts to feel a lot like doing your taxes: there's only one right answer, Remix knows what it is, but you're going to get quizzed on it anyway.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is a universal experience or more US-centric 😅

Choose a reason for hiding this comment

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

I can confirm that in Brazil it's exactly like that too

decisions/0012-type-inference.md Outdated Show resolved Hide resolved
decisions/0012-type-inference.md Outdated Show resolved Hide resolved
decisions/0012-type-inference.md Outdated Show resolved Hide resolved
@david-crespo
Copy link
Contributor

david-crespo commented Sep 23, 2024

Looking forward to this feature. I enjoyed reading the decision doc. I wasn't familiar with TS LSP plugins, so I looked them up and found this in the TS wiki:

TypeScript Language Service Plugins ("plugins") are for changing the editing experience only. The core TypeScript language remains the same. Plugins can't add new language features such as new syntax or different typechecking behavior, and plugins aren't loaded during normal commandline typechecking or emitting, (so are not loaded by tsc).

If this is true, then it seems like a big downside of the plugin approach that's worth addressing. If it is not true and tsc on the command line will turn up type errors from the generated types, then that would be worth mentioning as well. When I saw LSP plugin, I immediately wondered how it will work on the command line.

Edit: Ok, and that's why you don't comment on work in progress:

2. **TypeScript plugin do not affect typechecking**
TypeScript does have [language service plugins](https://github.com/microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin),
but these plugins can only augment the experience in your editor.
In other words, these plugins do not get used when `tsc` to typecheck your code.
Relying solely on your editor to report type errors is insufficient as type errors can easily slip through.
For example, you could edit one file and cause a type error in another file you never opened.
To ensure this never happens, typechecking needs to be a standalone command runnable by CI.

@pcattori
Copy link
Contributor Author

pcattori commented Sep 23, 2024

Looking forward to this feature. I enjoyed reading the decision doc. I wasn't familiar with TS LSP plugins, so I looked them up and found this in the TS wiki:

TypeScript Language Service Plugins ("plugins") are for changing the editing experience only. The core TypeScript language remains the same. Plugins can't add new language features such as new syntax or different typechecking behavior, and plugins aren't loaded during normal commandline typechecking or emitting, (so are not loaded by tsc).

If this is true, then it seems like a big downside of the plugin approach that's worth addressing. If it is not true and tsc on the command line will turn up type errors from the generated types, then that would be worth mentioning as well. When I saw LSP plugin, I immediately wondered how it will work on the command line.

Edit: Ok, and that's why you don't comment on work in progress:

2. **TypeScript plugin do not affect typechecking**
TypeScript does have [language service plugins](https://github.com/microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin),
but these plugins can only augment the experience in your editor.
In other words, these plugins do not get used when `tsc` to typecheck your code.
Relying solely on your editor to report type errors is insufficient as type errors can easily slip through.
For example, you could edit one file and cause a type error in another file you never opened.
To ensure this never happens, typechecking needs to be a standalone command runnable by CI.

For onlookers: its worth calling out that everything in this decision doc (#0012) keeps typechecking working as expected. The only things proposed in #0012 are automatically running typegen in watch mode and adding some editor DX features. As you point out (#0013) is where this limitation of TS plugins is relevant.

@david-crespo
Copy link
Contributor

david-crespo commented Sep 23, 2024

I think what threw me off is that the first paragraph of the Typegen section makes it sound like typegen is primarily about file-based route config.

### Typegen
While React Router will default to programmatic routing, it can easily be configured for file-based routing.
That means that sometimes route URLs will only be represented as file paths.
Unfortunately, TypeScript cannot use the filesystem as part of its type inference nor type checking.
The only tenable way to infer types based on file paths is through code generation.

But from doc 13 it sounds like the language service is still leveraging the typegen directory behind the scenes for programmatic routing, you just don't have to annotate the route exports yourself.

And the language service will make sure the typechecker sees this:
```tsx
// app/routes/product.tsx
// URL path: /products/:id
export function loader({ params }: import("./+types.product").loader["args"]) {
const user = getUser(params.id);
return { planet: "world", user };
}

@pcattori pcattori changed the title experimental typesafety phase 1 Typesafety Sep 25, 2024
@markdalgleish
Copy link
Member

The decision doc is really nice 👍

@pcattori pcattori marked this pull request as ready for review October 1, 2024 16:44
@pcattori pcattori changed the title Typesafety Typesafety improvements Oct 1, 2024
@pcattori pcattori merged commit 7ca1e40 into dev Oct 1, 2024
8 checks passed
@pcattori pcattori deleted the pedro/typesafety-phase-1 branch October 1, 2024 17:19
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.

8 participants