fix: Restores eager recursion into options
objects
#478
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
structure(list(), class = "custom_options"))
. This is a common way to create a list-like options argument. The problem is thatstructure()
doesn't include the"list"
class when theclass
argument is provided, and therefore the code introduced in Search forJS()
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...component
retainslist
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 thatJS()
could be used on htmltoolstag()
attributes, which could be at any part of the HTML tree, and where the outershiny.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 thatshouldEval()
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.