-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: allow params number #206
Conversation
src/router.ts
Outdated
? { ...rawLocation, params: encodeParams(rawLocation.params) } | ||
? { | ||
...rawLocation, | ||
params: encodeParams(applyToParamsRaw(rawLocation.params)), |
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.
A few thoughts for refactoring:
encodeParams
could accept numbers- we could create a copy of
rawLocation
before L256, and add an else at L261 to change theparams
property to an encoded version without numbers, save the decoded version as a local variableparams
to use it later
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.
encodeParams
is accepting string | number
but the bind
doesn't pick up correct overload, I will just create a const with the normalized
params and use that across
src/utils/index.ts
Outdated
export function applyToParamsRaw( | ||
params: RouteParamsRaw | undefined | ||
): RouteParams | undefined { | ||
return applyToParams(x => x.toString(), params) | ||
} |
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 think we could write this one as
export function applyToParamsRaw( | |
params: RouteParamsRaw | undefined | |
): RouteParams | undefined { | |
return applyToParams(x => x.toString(), params) | |
} | |
export let normalizeParams = applyParams.bind(null, paramValue => '' + paramValue) |
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.
noticed let
instead of const
, is that on purpose? If it is, why? :)
src/router.ts
Outdated
@@ -275,7 +278,7 @@ export function createRouter(options: RouterOptions): Router { | |||
// TODO: normalize params if we accept numbers as raw values | |||
matchedRoute.params = | |||
'params' in rawLocation | |||
? rawLocation.params! | |||
? applyToParamsRaw(rawLocation.params)! |
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.
we can remove the todo above
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #206 +/- ##
==========================================
+ Coverage 88.61% 88.64% +0.03%
==========================================
Files 25 25
Lines 1019 1022 +3
Branches 279 279
==========================================
+ Hits 903 906 +3
Misses 66 66
Partials 50 50
Continue to review full report at Codecov.
|
I ended up doing something simpler by not allowing number params for the matcher at ef0920a |
Allow have
param
with number as a value