-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Unified withParams
utility to replace individual path
, query
, and host
utilities
#447
Conversation
✅ Deploy Preview for kitbag-router ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/services/paramValidation.ts
Outdated
|
||
return { | ||
...getPathParams(route.host, `${protocol}//${host}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure this is a bug in production right now, host
advertises support for params but doesn't validate them when matching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be getHostParams
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we reuse "path" functions, rename without the word "path".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
investigate if we actually need a "query" version as well
src/services/withParams.spec.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose not to duplicate all the tests from path
, host
, query
, etc since the specific formats aren't expected to make a difference
src/services/combineHash.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the combinePath naming doesn't make sense now that its used for all the different pieces. Its also kinda interesting that since it accepts WithParams
that there's no type safety preventing us from accidentally doing WithParams<TParent['path'], TQuery>
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have that protection today, but it might be worth exploring. Something like assigning an internal kind
(tagged type) to ensure wires don't get crossed
src/services/combinePath.ts
Outdated
? WithParams<CombinePathString<TParentPath, TChildPath>, RemoveLeadingQuestionMarkFromKeys<TParentParams> & RemoveLeadingQuestionMarkFromKeys<TChildParams>> | ||
: WithParams<undefined, {}> | ||
: WithParams<undefined, {}> | ||
: WithParams<undefined, {}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we gain something from the undefined
vs {}
change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we just assume hash ''
(empty string) is the same as undefined
we could remove all the toStrings()
. See if you can remember why hash was special and expected undefined
separately from empty string
src/services/paramValidation.ts
Outdated
|
||
return { | ||
...getPathParams(route.host, `${protocol}//${host}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be getHostParams
?
src/services/routeMatchScore.ts
Outdated
|
||
return optionalParams.length - missing.length | ||
} | ||
|
||
export function countExpectedQueryParams(route: Route, actual: QuerySource): number { | ||
const actualQuery = new URLSearchParams(actual) | ||
const expectedQuery = new URLSearchParams(route.query.value) | ||
const expectedQuery = new URLSearchParams(route.query.toString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we go back to having a toString
method? Just for my own understanding.
withParams
utility to replace individual path
, query
, and host
utilities
We ship separate utilities for assigning params for each of the different parts of the url. For the most part these all worked exactly the same (Hash was a little different, more on that later). This PR introduces a single, unified utility
withParams
to replace them all.I really didn't like the fact that
hash
didn't quite fit but still had extremely similar types. The biggest difference is thathash
stored the value asstring | undefined
, but the others always storestring
. Hence our empty paths being defined asPath<'', {}>
. I actually prefer the idea of undefined sowithParams
stores value as possibly undefined. It has an overload that takes zero arguments and a separate propertytoString: () => string
for instances where we need to normalize to a string.Additionally, I decided to support params in the
hash
because it works exactly like path and I assumed it would be a small change. It's a bit bigger than I expected but still small enough that I feel like it was the right call.Hash also had some logic for stripping a leading "#" when it's created but I just moved that so it would work like the other utilities.
To be kind to our current user base I've exported
path
,query
, andhost
utilities but marked them as deprecated.