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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse params in TypeScript #8030

Merged
merged 5 commits into from Dec 9, 2021
Merged

Parse params in TypeScript #8030

merged 5 commits into from Dec 9, 2021

Conversation

kddnewton
Copy link
Contributor

Hi! 馃憢

I saw #8019 go out, which is great! useMatch should definitely be generic to make the types better. One thing that's nice though is that modern TypeScript supports template strings, so these types can actually be parsed statically if they're string literals. This commit makes it attempt to parse the type statically and if not, it falls back to just string. You can play around with the parsing here.

Now when you use useMatch("foo/:bar") the type returned is actually PathMatch<"bar"> and useMatch("foo/:bar/:baz") is PathMatch<"bar" | "baz">.

Totally get it if you don't want to maintain this, but I think it's really nice on the consumer side because you don't have to duplicate that key.

Tagging @chaance since you made that original PR that inspired this one.

@mjackson
Copy link
Member

WHAT MAGIC IS THIS?! Haha, I've never seen anything like this. Very cool, @kddnewton. Thanks for bringing this to our attention.

I could go either way with this. I think I understand what's going on in the code (thanks for the comments!) so I'd be fine maintaining it if people really want it. What do you think @chaance?

I think we may also need to do a bit more work here to augment the return value of matchPath the same way as useMatch, right?

@smashercosmo
Copy link
Contributor

smashercosmo commented Sep 13, 2021

Would be also nice to have exported Params type, that could look like

type Params<Path extends string> = Record<ParseParamKey<Path>, string>

so that we can use it with useParams() hook, like

import { useParams } from 'react-router-dom'
import type { Params } from 'react-router-dom'

function UserPost() {
  const { userId, postId } = useParams() as Params<'/users/:userId/posts/:postId'>
}

@smashercosmo
Copy link
Contributor

smashercosmo commented Sep 13, 2021

Oh, I've just realized, that it's not needed, because of the latest changes to useParams hook. So, just exported ParseParamKey type will be enough.

import { useParams } from 'react-router-dom'
import type { ParseParamKey } from 'react-router-dom'

function UserPost() {
  const { userId, postId } = useParams<ParseParamKey<'/users/:userId/posts/:postId'>>()
}

@chaance
Copy link
Collaborator

chaance commented Sep 13, 2021

@mjackson love this, I actually wanted to do something similar but didn't spend the time on trying to figure out the patterns, but I'm very glad @kddnewton did. I say we do it!

@kddnewton
Copy link
Contributor Author

@mjackson I updated matchPath to work the same way.

Yeah, template strings are ridiculous. I'm gonna leave some links here in case people find this issue and get curious:

@chaance
Copy link
Collaborator

chaance commented Sep 13, 2021

@kddnewton Meant to echo Michael on this, but thanks so much for the inline comments. This kind of stuff is very helpful for us and anyone who contributes after us 馃檹

@kddnewton
Copy link
Contributor Author

@mjackson let me know if there's anything else you'd like me to do for this to go out, and thanks for being open to it!

@smashercosmo
Copy link
Contributor

could you please consider exporting ParamParseKey type

@kddnewton
Copy link
Contributor Author

Sure @smashercosmo I've added it as exported. I wasn't sure if maintenance of that public type was preferable, but I suppose you could derive it anyway.

@smashercosmo
Copy link
Contributor

Thank you) though we should've probably waited for @mjackson response. There is chance that he might not want to make it public. But as I said before it will be very convenient to use it with useParams hook

import { useParams } from 'react-router-dom'
import type { ParseParamKey } from 'react-router-dom'

function UserPost() {
  const { userId, postId } = useParams<ParseParamKey<'/users/:userId/posts/:postId'>>()
}

@kddnewton
Copy link
Contributor Author

Just rebased in case y'all get a minute to check this out. <3

@chaance
Copy link
Collaborator

chaance commented Nov 11, 2021

Pinging @mjackson on this but I'd prefer we didn't export that type. It's more of an internal utility and I could easily see us needing to change or remove it before v7 if TypeScript releases a new feature we can leverage to optimize it further.

@kddnewton
Copy link
Contributor Author

No problem @chaance, I've unexported it now and rebased.

I saw #8019 go out, which is great! useMatch should definitely be generic to make the types better. One thing that's nice though is that modern TypeScript supports template strings, so these types can actually be parsed statically if they're string literals. This commit makes it attempt to parse the type statically and if not, it falls back to just string. You can play around with the parsing here: https://www.typescriptlang.org/play?ts=4.4.2#code/C4TwDgpgBACghgJzgW3ggzhAYnAlgGwgBMoBeKAbyggQQHsEAuKYBAV2gF8BuAWACgBoSLEQo0mADwBlCAHNkEAHbBqAD2DKi6KOla4lcgHxkBUKAHoLUAMIALCAGMA1lAcJouAGYt30CGq4ejpwUF4MAO6IJOj4cOh2UAa+0HoIBnIAdGZQsgrKqgGaStpQAAYAJBQGXjRQADIQXsB5iiqcFlU1dQBKuHJ2LfJtwJxlOeZWUACSPsB+SSFhkdG6cQkANClKUHDAmshgqsB0UGCImNRwjonouETQdHMOE5bW4QhRCDHrdtn85nMAH5REhUBcIJJGs1WgUTEUtDpuggGk1gD0IOg2PhgK9gaDxBDJH0BkN8ip4RpEUklLUUSTBhisTi8YCQdD0ZjsYUqSUdGkMqzAW8Zs9oIRmrp7p5+WxHI5MegvNj8CAzhCSPFduqwVt5sooDcnK5vELAVN9VB0qSpQ9DXAdgAjVJyhXoJUqtXnDDEXY6CIQfD4TKiqCOujzW2Ys2TayOB1h6DezBEPUOHaWjzM1RBbVsJS4Og7J4pFgRU53B7oGMigAUHjgRGYACJwnRm1AAD5QZuOxDNgCU-2FI6gIIZnOz6mKpQFhjHqOaTO5XagE+XOKgzA5G9xANH5uss1LEtUlZlUCUEd0rsVyqDXo1fu13pQaYNRpcNam3igRdVpbWoMUb2k6Lryneno6pcWqhK+yAhseuB7geh6ge+GYOFaXKbrmABWbB6Fa-SDFsgaXMhADkSxeHg+BsB4w6oVua4kZOK4Inyuj6PO45sbuLFoISPo4AQxBmswQngiJdHifuwpTMelqniBRD3EolGqMmngqKccFiMgGFQARRGfq4mZsayUznkxwqSQZEiQhOsIUtO1LIqxpK7mafFeThPIzvyPFyAu67+YJDkQqJhBEBJBLSZg0VyQpR5ih41GXqcHxfD88R2EZZlJM8uaYOSZ7AIgwD+shiRwK8UyOHQ+BFohPjIVARB0JiRnzLmuahEQIBKCguCOLowwFFs6CnJm4VBPV1gmccWEeMgeAlHUJaWnOWRQAA8vqnxBBAWxUTRdEMRAtksS5AXUmUjBdLSvQQGtBgZGMoWvetGQRWCjlJUQfCCPwwjQFJ6AyBNKhuVxO0mKQORSY5UNlZSgXcekhgLaG23Q6o8ZgTeEHuveAHaSQBgnNqmCqCWO1QPgyE0HA+DoBsOP6ph0BZiuuZyHQv3U862prWAf7PHQlw7SEOjOBAIAhCUONgFLdyOgB+YPF4Bi+jLz6WgAbqzHDoNdIJoMAuCs8STgMEQkjI0St1GFNwVGEYONKVhpUjKBV6qCLWIkx6D7QcQRkBpRHjGYRqgC0LdA43YcCG9AoQC3QJDa3AK60UGfYuBLNPBVAAC0Jg7SCYvXfZCBWzbGKNd8kg7W7WPGEYwMCAEqv12E+aOFbRZQIREAALJ7DcMiw7OwVbDAs86BDMge7W8HoMw0gDpJlA5B4wAMTsFCcM+MDA5wAgCGPk-ADctatnQdAWH2CAv3AABeg7AzfU92A-bYLCMFfu-L+A4f6YFvvfR+z9gGICAX2MBECJ5-wfowQBcC36IO-tfSBqDmzoKfgg+BcCkECCAA.
@ryanflorence
Copy link
Member

@kddnewton I fixed the merge conflicts, you wanna put your username in contributors.yaml to sign the CLA and I'll merge this puppy?

@ryanflorence ryanflorence added the accepted Accepted the changes, waiting for tests/CLA/etc. label Dec 9, 2021
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 9, 2021

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

@kddnewton
Copy link
Contributor Author

@ryanflorence absolutely!

@ryanflorence
Copy link
Member

When there are bugs, I'm pinging you cause I have no clue what's going on in there 馃槤 but the typescript sandbox was awesome! Too bad we can't do that for useParams().

@kddnewton
Copy link
Contributor Author

Hit me up anytime, happy to help! :)

@timdorr
Copy link
Member

timdorr commented Dec 9, 2021

Formatting is fixed up for you too 馃憤

@ryanflorence ryanflorence merged commit 5a54d76 into remix-run:dev Dec 9, 2021
@ryanflorence
Copy link
Member

Thansk @timdorr, I've been over here trying to get github.dev to use the project prettier config but it won't!

@timdorr
Copy link
Member

timdorr commented Dec 9, 2021

Yeah, Prettier doesn't work for me on github.dev either :/

@kddnewton kddnewton deleted the parse-params branch December 10, 2021 18:21
@kddnewton
Copy link
Contributor Author

Woohoo! Thanks for the merge! Excited about this.

brophdawg11 pushed a commit that referenced this pull request Mar 27, 2024
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the changes, waiting for tests/CLA/etc. CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants