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

Explicitly register knit_print S3 methods #108

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

wch
Copy link
Collaborator

@wch wch commented Aug 23, 2018

This PR un-exports the knit_print methods and registers them as S3 methods when both knitr and htmltools are loaded.

With R >=3.5.0, htmltools must be attached in order for the methods to work. There's an exception when shiny is attached: because shiny imports and re-exports these methods, they will also work if shiny is attached.

One consequence is that, because htmltools no longer exports these functions, Shiny can't import them. So Shiny will need to be updated to stop doing that.

Note that the CRAN version of Shiny (1.1.0) cannot be installed with this version of htmltools -- Shiny will fail to build because it can't import the knit_print methods from htmltools. If this version of htmltools is installed with an existing Shiny 1.1.0 installation, I believe that Shiny will no longer be able to be loaded.

@jcheng5
Copy link
Member

jcheng5 commented Aug 24, 2018

Note that the CRAN version of Shiny (1.1.0) cannot be installed with this version of htmltools -- Shiny will fail to build because it can't import the knit_print methods from htmltools. If this version of htmltools is installed with an existing Shiny 1.1.0 installation, I believe that Shiny will no longer be able to be loaded.

Will Shiny 1.2.0 be able to work with both older and newer htmltools? Or do we really need to do a completely synchronized pair of releases?

@wch
Copy link
Collaborator Author

wch commented Aug 24, 2018

Here are the combinations:

  • Old shiny, old htmltools: this is the current state of the world. knit_print methods are available if either shiny or htmltools is attached.
  • New shiny, new htmltools: everything works -- knit_print methods work even if neither shiny nor htmltools are attached.
  • New shiny, old htmltools: shiny works, but knit_print methods are available only if htmltools is attached. (Attaching shiny doesn't make them available.) Note that in the Shiny PR (Un-export knit_print methods from htmltools shiny#2166), I bumped the htmltools dependency so that this case shouldn't ever happen.
  • Old shiny, new htmltools: Shiny will not build in this case. If it is already installed and htmltools upgraded, then shiny will fail to load.

So there is a problem with the upgrade path. We can't release htmltools first because it breaks shiny. If we released Shiny first (I could remove the htmltools version bump), then knit_print could be broken for users.

Maybe the best solution is to continue to export the knit_print methods for now, in addition to the registration; then we release new Shiny (which doesn't re-export them), then at some point in the future, after we can stop exporting them from htmltools.

@wch
Copy link
Collaborator Author

wch commented Aug 24, 2018

I've changed this PR so that the only change is the the explicit registration of S3 methods; the knit_print methods are still exported. This avoids the shiny/htmltools upgrade issues described above.

@wch wch merged commit 8cf2579 into rstudio:master Aug 24, 2018
@wch wch deleted the unexport-knit-print branch August 24, 2018 17:54
@wch wch changed the title Un-export knit_print methods Explicitly register knit_print S3 methods Aug 24, 2018
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.

2 participants