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

Handle certain NULL query results #74

Merged
merged 1 commit into from
May 15, 2020

Conversation

woahdae
Copy link
Contributor

@woahdae woahdae commented May 13, 2020

Previously, if the Keen API responded with a result having a null entry,
parsing the response would result in a javascript error when this
project checks to see if the result needs to be cast to a Number.

This checks a few possible places were the API might return null, and does
not attempt to cast in these cases. This means null gets passed to the
chart, but in practice this hasn't been an issue. If it is, it could be
handled at the server level or the charting level, but at least this
project won't crash on it.

Note that only line 123 of current master is failing for me, but this commit changes three places total. The area surrounding line 123, and two other areas that look very similar in roughly the same part of the codebase. These other areas are total guesses, but since clearly this code doesn't expect the API to return null, maybe it's worth doing. 🤷‍♂️I did not try to get every possible place where something is returned from the API here.

Here's a couple screenshots to show what the data looks like to me when this happens. Only one null, right at the end:

Screen Shot 2020-05-12 at 5 35 24 PM

Screen Shot 2020-05-12 at 5 34 58 PM

Previously, if the Keen API responded with a result having a null entry,
parsing the response would result in a javascript error when this
project checks to see if the result needs to be cast to a Number.

This checks a few possible places were the API might return null, and does
not attempt to cast in these cases. This means null gets passed to the
chart, but in practice this hasn't been an issue. If it is, it could be
handled at the server level or the charting level, but at least this
project won't crash on it.
@woahdae
Copy link
Contributor Author

woahdae commented May 13, 2020

It's also worth noting that I can reproduce the issue in the console. Here, the 7 day timeframe does not have the extra null value at the end, where the 30 day timeframe does, and you can see the console doesn't handle it either.

[Update - fixed the broken gif]

ezgif-5-0a961ad02129

@maciejrybaniec maciejrybaniec merged commit a802009 into keen:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants