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

insure component retains list #82

Closed
timelyportfolio opened this issue Nov 24, 2023 · 4 comments
Closed

insure component retains list #82

timelyportfolio opened this issue Nov 24, 2023 · 4 comments

Comments

@timelyportfolio
Copy link
Collaborator

As pointed out in ramnathv/htmlwidgets#477 and glin/reactable#348, htmlwidgets::JS will no longer work as expected in reactR::component. I think @glin fix in glin/reactable@1d60ea6 might be the solution that we could make universal if applied in reactR.

timelyportfolio added a commit that referenced this issue Nov 24, 2023
…lwidgets` will recurse through extracting `htmlwidgets::JS` calls
@gadenbuie
Copy link

Hi @timelyportfolio, thanks for the fix here and apologies for the disruption. reactR's use of JS() was definitely not something I anticipated when working on the PR that changed the recursive behavior of htmltools::shouldEval().

Would you be able to show me in a small example how reactR uses JS()? It would help me to understand the scope of the problems this new behavior is causing.

@timelyportfolio
Copy link
Collaborator Author

timelyportfolio commented Dec 2, 2023

@gadenbuie thanks so much for working through this. Happy to schedule a time to discuss if that works best. My best summary of how reactR uses JS() is that a reactR::component is a thin wrapper of shiny.tag. Any reactR widget is nearly identical to an htmlwidget except there might be component instead of shiny.tag.

I will likely preserve this change regardless of decision in htmlwidgets, since I do not see any negative side effects of preserving list. @glin what are your thoughts?

@glin
Copy link

glin commented Dec 11, 2023

@gadenbuie @timelyportfolio Thanks for fixes both in htmlwidgets and reactR. I don't see any downsides to leaving the workaround in either, especially because there are likely still users on htmlwidgets 1.6.3 out there, and it's kind of a hard issue to spot. I will probably leave in the workaround for reactable a little longer as well.

@timelyportfolio
Copy link
Collaborator Author

thanks @glin and @gadenbuie I'll plan to leave the patch to add list to set of classes for reactR::component.

timelyportfolio added a commit that referenced this issue Jun 28, 2024
`list` class #82, hydrate tags in attribs #67, switch to `vite`/`vitest`
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

No branches or pull requests

3 participants