Skip to content

experimental typesafety phase 2 #12022

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

Closed
wants to merge 13 commits into from
Closed

experimental typesafety phase 2 #12022

wants to merge 13 commits into from

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Sep 20, 2024

Depends on #12019

nothing to see here, move along

TODO

  • docs
  • decision doc
  • changeset
  • cache by root dir
  • no "app/" dir assumption
  • tests
  • handle re-exports

Copy link

changeset-bot bot commented Sep 20, 2024

⚠️ No Changeset found

Latest commit: 2d5e406

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

@pcattori pcattori force-pushed the pedro/typesafety-phase-2 branch from a9a8aae to f9325ab Compare September 21, 2024 05:44
@pcattori pcattori force-pushed the pedro/typesafety-phase-2 branch from f9325ab to f20414d Compare September 23, 2024 15:22
@pcattori pcattori force-pushed the pedro/typesafety-phase-2 branch from f20414d to 2d5e406 Compare September 23, 2024 20:07

For other APIs you don't own — like third party libraries — you expect types to be provided for you.
That way, you can stick to annotating your own APIs when necessary but let type inference do the heavy lifting everywhere else.
So it feels unnatural to do so for React Router's route export API.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is a little awkward, maybe something like

Suggested change
So it feels unnatural to do so for React Router's route export API.
That's why it feels unnatural to annotate React Router's route export API.

Comment on lines +149 to +150
Remix and React Router already use Vite to compile and bundle your code.
So delegating typechecking to TypeScript programmatically is less of a departure from `tsc` than using Vite is.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be 1 sentence

Suggested change
Remix and React Router already use Vite to compile and bundle your code.
So delegating typechecking to TypeScript programmatically is less of a departure from `tsc` than using Vite is.
Remix and React Router already use Vite to compile and bundle your code, so delegating typechecking to TypeScript programmatically is less of a departure from `tsc` than using Vite is.

Comment on lines +179 to +180
One reason you might want to do this is if your project uses tools that independently inspect types in your app.
For example, [typescript-eslint](https://typescript-eslint.io/) will think that route export args are typed as `any`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the implications of this exactly, but my understanding is this is a very popular package, so I'm a bit nervous about this disclaimer. Not sure if there are patterns or an eslint plugin or something we can provide to help smooth adoption, but as it's written this basically sounds likes

if you want to use typescript-eslint you don't get automatic typing at all, you have to manually type everything

Is that correct?

@pcattori
Copy link
Contributor Author

pcattori commented Sep 25, 2024

closing as we think we have a better, simpler approach that doesn't require injecting types for route modules and has better interop with other tools. still fun to see how far this experiment could push things!

@pcattori pcattori closed this Sep 25, 2024
@pcattori pcattori deleted the pedro/typesafety-phase-2 branch September 25, 2024 21:04
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.

2 participants