Update parseInterval to handle "0" correctly #1835
Conversation
When a parameter like "0ms" is passed in to parseInterval it gets parsed to 0. Previously this would result in a return value of "undefined" because 0 is falsy and thus the `return 0 || undefined` statements return undefined. The purpose of the form `parseFloat(str) || undefined` was to return "undefined" if parseFloat failed (parseFloat returns NaN, a falsy value, if it can't parse its argument). Unfortunately, as mentioned, parseFloat can also succeed and return a falsy value -- when the argument is "0" (or "0.0", etc.). So the new code, rather than depending on the falsiness of the result of parseFloat, explicitly checks for a NaN.
Adds some semicolons to parseInterval (and tests) for consistency.
Adds test test to make sure parseInterval works on "0".
|
Good catch! Falsy checks indeed become an issue when 0 is an acceptable value
|
Telroshan
added
ready for review
|
Now that I think about it @tncowart Could you add tests with 0 values to make sure this actually fixes the end behavior? Internally, htmx uses |
|
@Telroshan Do I need to update tests in both |
|
@tncowart The |
|
@Telroshan Are these sufficient? I also updated one of the falsy conditionals in htmx.js. |
|
Thanks a lot for the tests @tncowart! |
These values come from user settings that are read from parseInterval,
so they could be a number or undefined.
If the value being checked is > 0 setTimeout will be called with some
associated function. If the value is 0 or 'undefined' the associated function
will be called immediately ('undefined' is not greater than 0).
There was a problem hiding this comment.
In a7c9bcd
Why not use the !== undefined check here as you did for the pollInterval?
As per the setTimeout doc,
delay
If this parameter is omitted, a value of 0 is used, meaning execute "immediately", or more accurately, the next event cycle.
So having a delay of 0 is not the same as calling the function right away, as it'll in fact execute in the next cycle
Let me know if I'm missing something there!
|
I think checking for x > 0 keeps the same behavior as before (0 and undefined are both false), whereas checking for x !== undefined subtly changes the behavior (only undefined is false). So maybe I should do > 0 everywhere, instead of !== undefined. Does that sound right -- change the !== undefined test to > 0? |
|
Oh you're right, it could be a breaking change as what was formerly called immediately would suddenly be delayed to the next cycle I see that the default swap delay is set to 0 in the default config, so it would impact all existing apps using htmx, forget that! |
`pollInterval !== undefined` is a subtly different conditional than just `pollInterval` or `pollInterval > 0` (which are equivalent). Changes the conditional to `pollInterval > 0` so as to not change the behavior but also be more explicit in the test.
alexpetros
added
ready for review
When a parameter like "0ms" is passed in to parseInterval it gets parsed to 0. Previously this would result in a return value of "undefined" because 0 is falsy and thus the
return 0 || undefinedstatements return undefined.The purpose of the form
parseFloat(str) || undefinedwas to return "undefined" if parseFloat failed (parseFloat returns NaN, a falsy value, if it can't parse its argument). Unfortunately, as mentioned, parseFloat can also succeed and return a falsy value -- when the argument is "0" (or "0.0", etc.). So the new code, rather than depending on the falsiness of the result of parseFloat, explicitly checks for a NaN.