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

fix: Restores eager recursion into options objects #478

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

gadenbuie
Copy link
Collaborator

Given the disruption caused by #467 and the innumerable ways that people may have unknowingly been relying on htmlwidget's previously eager recursion into any list-like object, this PR restores previous behavior.

Both #466 and rstudio/DT#1092 provide examples why shouldEval() shouldn't recurse into arbitrary objects -- while uncommon, it's possible to run into cases where an object is infinitely recursive.

On the other hand, rstudio/leaflet#891 and react-R/reactR#82 show two cases where eager recursion was a feature, not a bug:

  1. In htmlwidgets 1.6.3 breaks addEasyButton rstudio/leaflet#891, issues arise from using something akin to structure(list(), class = "custom_options")). This is a common way to create a list-like options argument. The problem is that structure() doesn't include the "list" class when the class argument is provided, and therefore the code introduced in Search for JS() calls recursively only in explicit lists and data.frames #467 would not recurse into these objects. We could do one level of recursion for list-like, but not "list" inherting, objects, but...
  2. insure component retains list react-R/reactR#82 shows that a custom, non-list classed object isn't a reliable signal of how far a package author expects htmlwidgets to recurse. In reactR's case, AFAICT, there's an expectation that JS() could be used on htmltools tag() attributes, which could be at any part of the HTML tree, and where the outer shiny.tag doesn't include a "list" class.

This PR will at least fix #477 and likely many other places we have yet to discover.

While it seems reasonable to carve out an exception for POSIXlt as in #473, both that PR and #466 indicate that there are likely many object classes that could require an exception. It's not clear to me that htmlwidgets should go down the path of carving out specific exceptions, especially considering that shouldEval() is called on an object that is in the process of moving from R to JSON, where these objects need to be serialized as common JSON types anyway.

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update NEWS as well please, thanks!

@gadenbuie gadenbuie merged commit dec6c30 into master Nov 27, 2023
8 checks passed
@gadenbuie gadenbuie deleted the fix/js-recursive-should-eval branch November 27, 2023 21:02
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.

Call "JS function" Incompatibility with other package (reactable)
2 participants