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

onClick: Missing values (NA) return different values in numeric vs other column types #261

Closed
daattali opened this issue Aug 5, 2022 · 3 comments
Labels
bug Something isn't working
Milestone

Comments

@daattali
Copy link

daattali commented Aug 5, 2022

See the example below:

df <- data.frame(
  numeric = as.numeric(c(1, NA)),
  logical = as.logical(c(1, NA)),
  character = as.character(c(1, NA)),
  factor = as.factor(c(1, NA))
)
reactable::reactable(
  df,
  defaultColDef = reactable::colDef(
    na = "–"
  ),
  onClick = reactable::JS("
        function(rowInfo, column) {
          alert(rowInfo.row[column.name]);
        }"
  )
)

Clicking on the missing value in the first column will say "NA", whereas in the other columns it'll say "null". This is problematic because in order to differentiate true NA from the string "NA" we need to look at the column type. It would be better if "null" was always returned.

{reactable} v 0.3.0

@glin
Copy link
Owner

glin commented Aug 21, 2022

Thanks for the report. Previously I would've considered this an undocumented wart, but I agree that using null for numeric NAs would be so much better, especially since NA is represented as null for every other column type.

From vague memory, I think this was originally intentional because of jsonlite's handling of numerics with toJSON(na = "null"). If you use na = "null" to serialize NA as null, other non-NA numeric values get lost and converted to null for some reason:

> jsonlite::toJSON(c(1, 2, NA, NaN, Inf, -Inf))
[1,2,"NA","NaN","Inf","-Inf"]

> jsonlite::toJSON(c(1, 2, NA, NaN, Inf, -Inf), na = "null")
[1,2,null,null,null,null] 

Although there's no NaN or Inf in JSON, I still would've expected these to remain as strings and not converted to nulls.

So for a fix, the best path might be to leave the JSON serialization as-is but convert the NA/NaN/Inf strings to null/NaN/Inf on the JavaScript side, which would make NaN/Inf values easier to work with as well.


Since this will technically be a breaking change, a way to check for numeric NAs that would work both pre- and post-fix would be something like if (typeof value === 'number') { /* value is a number */ } else { /* value is NA */ }

@glin glin added the bug Something isn't working label Aug 21, 2022
@daattali
Copy link
Author

Thanks for looking into this. For completion, my current fix is:

if (value === null || (value === 'NA' && column.type === 'numeric') { // missing value }

@glin glin added this to the v0.4.0 milestone Nov 27, 2022
@glin
Copy link
Owner

glin commented Nov 27, 2022

This is now fixed in 2fc979b:

Breaking changes

  • Numeric NA values are now represented as null in JavaScript instead of an "NA" string.
    Numeric NaN, Inf, and -Inf values are now represented as NaN, Infinity, and -Infinity
    in JavaScript instead of "NaN", "Inf", and "-Inf" strings. (@daattali, #261)
    function(cellInfo) {
      // Old
      cellInfo.value // "NA", "NaN", "Inf", "-Inf"
      // New
      cellInfo.value // null, NaN, Infinity, -Infinity
    }

@glin glin closed this as completed Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants