Skip to content

doNotParse: Undocumented, often unwanted behavior #63

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Domiii opened this issue Apr 28, 2017 · 0 comments · Fixed by #65
Closed

doNotParse: Undocumented, often unwanted behavior #63

Domiii opened this issue Apr 28, 2017 · 0 comments · Fixed by #65

Comments

@Domiii
Copy link
Contributor

Domiii commented Apr 28, 2017

For some reason, equalTo query arguments sometimes get parsed as integers, even if they are not integers, leading to troublesome side-effects.

I used orderByChild in combination with equalTo on user ids, sending me on a two-hour bug hunt where the query worked for some users and not work for others; only to find out that for this query combination, the equalTo handler in actions.js, by default, parses their parameter as integer using parseInt, like so:

case 'equalTo':
  let equalToParam = !doNotParse ? parseInt(param[1]) || param[1] : param[1]

This sort of logic has multiple problems:

  1. It does not work for strings that begin with integers, such as firebase-generated ids: parseInt('11asfkvvjjjj--add') actually returns 11. That's right, parseInt ain't a quitter; if it doesn't work for the whole thing, it settles for any number prefix in the string.
  2. It does not work for 0: parseInt("0") || "0" actually returns the string "0" instead of number 0.

I propose a simple fix, borrowed from this StackOverflow discussion on integer string validation:

function isIntString(val) {
    return val === "" + ~~val;
}
// ...
let equalToParam = (!doNotParse && isIntString(param[1])) ? parseInt(param[1]) : param[1];

It's not perfect but certainly a lot better. Maybe just working with numbers in general, rather than int only would be better?

PS: There are a few more questions related to the rationale behind this design decision:

  1. You only assume that integers need parsing. What about floats or other sorts of special values that might have accidentally ended up string-encoded?

  2. And also: why is doNotParse not the default? Shouldn't special parsing behavior that might make your queries not behave the way they would when using vanilla firebase, be opt-in, rather than opt-out?

tiberiuc added a commit that referenced this issue May 13, 2017
fix #63 (doNotParse behavior in source/actions.js)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant