Update widget_html

[thebioengineer]

Addresses #376

Suggested two updates to widget_html that are implemented here:

• To behave like an s3 method where it will look up the "type" of method it should use. This uses the "name" field to determine. Minimal changes from current name_html method.
• Add an object output type checker for custom functions to make sure they return a shiny.tag. Throws a warning describing what it saw and what it expected.

We could add a soft-deprication for the current method (name_html) if you want. I wasn't sure how we might want to do that.

• Add a check for the 'current' method and throw a warning? That might alarm users as opposed to the authors though
• Open to ideas!

[thebioengineer]

Should I add myself as a contributor?

[thebioengineer]

@jcheng5 - does this PR satisfy what you were thinking WRT #376? Or should I add in the soft-deprecation of the current behavior?

[jcheng5]

Sorry I missed this earlier, @thebioengineer.

I misread your original suggestion in #376, what I thought you were proposing was:

1. Turn widget_html into an actual S3 generic
2. Today's widget_html becomes widget_html.default (with the addition of checking the return value)
3. Any widgets that want to override widget_html can either use the old way, or preferably, define a widget_html.yourclassname method

This way, there's no breakage in backwards compatibility, and it's using the actual S3 generic dispatch mechanism instead of our own lookalike.

The harder question is when and how we would deprecate the old way, I think a PR like the above would be the first step which would allow us to update our docs. The next step would be to search CRAN reverse imports/dependencies and @timelyportfolio's repos to see if anyone is actually using the old way.

Sorry for the miscommunication! And let me know what you think of this proposed approach.

[thebioengineer]

Gotcha! Yeah, we can do that. I'll work on that tonight.

[timelyportfolio]

@jcheng5 @thebioengineer I agree this should be S3 and your idea is a more robust solution, but we should for now maintain backward compatibility. I do use *_html but is an easy change. I will also change the template in reactR once we have a new htmlwidget release. I know @glin will likely be interested as well https://github.com/glin/reactable/blob/master/R/reactable.R#L698.

[thebioengineer]

hey @jcheng5, I update html_widgets to behave like an s3 method. To facilitate the actual s3 method dispatch, I created a wrapper function - html_widget_wrapper. It also does the type checking we discussed.

Not sure I like this solution over our pseudo s3 dispatch method because I now we will have to export the methods, and so will the widget authors.

I added .Deprecated() to warn users about the changing methodology, but we can remove that if we want to hold off.

I checked this with both {reactable} and {leaflet} and it appeared to work still.

[jcheng5]

Other than my nit picky comment, this looks great now, thanks! I don't see exporting the generic (or S3method-ing the methods) as a problem, this is a totally mainstream use of S3, AFAICT.

[jcheng5]

Sorry, I just noticed this little detail, give me a second to get my head around this.

[thebioengineer]

AFAIK s3 methods are deployed based on the class of the object into UseMethod. This sets that. Otherwise we need to do our own s3 deployment method. unless there is another way to decide which s3 method to use?

[jcheng5]

The problem with setting the classname on the name is that it's a lie; we shouldn't tag objects with class names unless they actually are objects of that class. I didn't look closely enough at this initially and just assumed an x object was being passed in and that would be what we use for S3 dispatch. I had forgotten that this widget_html can be called before any x object exists--namely, in shinyWidgetOutput, where we're just generating the markup which won't actually be populated with the widget until the server renders it.

So that being the case, I don't think it makes sense to use actual S3 dispatch, I'm fine with the pseudo-S3 approach.

Sorry for the unnecessary extra work. The first commit was pretty close, I'll cherry-pick from this PR and probably file a new one to hopefully be merged next week.

[thebioengineer]

No worries, It was a good thought exercise as I went through this. I wasn't super clear about my intention for a "pseudo" s3 method implementation, just that I thought something similar might be more "R-like" and less likely to introduce someone stumbling across the widgetname_html method and getting weird results. Thanks for the review Joe!

[thebioengineer]

@jcheng5 - assuming this PR can be closed since your branch has been merged into master?