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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NaN bug with query/planParamsToQuery util #50

Closed
randolphjones opened this issue Jan 29, 2020 · 0 comments · Fixed by #57
Closed

NaN bug with query/planParamsToQuery util #50

randolphjones opened this issue Jan 29, 2020 · 0 comments · Fixed by #57
Assignees
Labels
bug Something isn't working MEDIUM-HIGH

Comments

@randolphjones
Copy link

randolphjones commented Jan 29, 2020

We found a bug with the planParamsToQuery function inside of v0.0.12 of core-utils

if (!Number.isNaN(params[key])) query[key] = parseFloat(params[key]);

Strings passed to the default case in this function are being turned into NaN. Having compared this to the OTP-RR code it looks like the global isNan was swapped with Number.isNaN when being added to OTP-UI's core utils. Number.isNaN will not implicitly coerse strings.

Worth noting that empty string values passed to the default condition still return NaN even with the global version of isNaN being used. We should be able to gracefully handle this condition in url query params

There are a couple of ways to address this. Feel free to reach out if you need some ideas or guidance. I think the best solution here will be to coerce the value with to a number before checking it with Number.isNaN

If this is the way it's intended to work then we can double-check this after the utility that constructs the OTP routing params has been released.

Thanks!

@fpurcell fpurcell added bug Something isn't working HIGH High Priority MEDIUM Medium Priority labels Jan 30, 2020
@fpurcell fpurcell added MEDIUM-HIGH and removed HIGH High Priority MEDIUM Medium Priority labels Jan 30, 2020
evansiroky added a commit to ibi-group/otp-ui that referenced this issue Feb 4, 2020
@evansiroky evansiroky mentioned this issue Feb 4, 2020
@fpurcell fpurcell added AS1 and removed AS1 labels Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working MEDIUM-HIGH
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants