-
Notifications
You must be signed in to change notification settings - Fork 4
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
Remove query param from URL when its value is default #181
Conversation
a1dba21
to
dfcfdb2
Compare
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 like this! Just a question about the implementation that I think is important to get right.
@@ -85,6 +85,8 @@ module QueryParams = { | |||
| Boolean => `switch ${variableName} { | true => "true" | false => "false" }` | |||
| Int => `Int.toString(${variableName})` | |||
| Float => `Float.toString(${variableName})` | |||
| CustomModule({moduleName, required: true}) => | |||
`${variableName} == ${moduleName}.defaultValue ? None : ${variableName}->${moduleName}.serialize->Some` |
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 have a better solution, but I don't think ==
works well enough to be used in this context. IIRC there are a few issues around newer representations we've implemented. I could be wrong though.
The only other way I can think of to do it is to serialize before comparing. That'll give a stable comparable value. It could be hurtful to performance if serializing is expensive, but that's probably rarely the case, and it's not done often. At that point one could consider caching the serialization of the default value, or something along those lines.
What do you think about serializing before comparing?
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.
It'd make sense, anyway we'd serialize the value without this feature, so it's the same complexity!
dfcfdb2
to
becb82c
Compare
becb82c
to
842e52a
Compare
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.
Looks great! Just one small nit regarding the comparison operator.
...ples/client-rendering/src/routes/__generated__/Route__Root__Todos__ByStatusDecoded_route.res
Outdated
Show resolved
Hide resolved
ad71ae3
to
503d151
Compare
to rebase on main once #180 is merged