rstudio / htmltools

Tools for HTML generation and output
https://rstudio.github.io/htmltools/
214 stars 67 forks source link

`includeHTML()` warns if complete document detected #382

Closed gadenbuie closed 1 year ago

gadenbuie commented 1 year ago

Fixes #124 by emitting a warning when includeHTML() is passed a path to a file containing a complete HTML document.

This PR originally contained an implementation of includeHTMLDocument(). I'll open a new issue to track progress on that front, but I didn't want to hold up this change on working out the details we'd need to address there.

Example

includeHTML() now warns with a recommendation to use includeHTMLDocument()

# Create a temporary complete HTML file
tmp_html <- withr::local_tempfile(fileext = ".html")
save_html(p("Test"), tmp_html)

includeHTML(tmp_html) |> print(browse = FALSE)
#> Warning message:
#> `includeHTML()` was provided a `path` that appears to be a complete HTML document.
#> ✖ Path: /var/folders/yl/0xhq7c7n3z7_9hp1356kctbr0000gp/T//Rtmp9wJCqR/filee0569372d04.html
#> ℹ Use `tags$iframe()` to include an HTML document. You can either ensure `path`
#>   is accessible in your app or document (see e.g. `shiny::addResourcePath()`)
#>   and pass the relative path to the `src` argument. Or you can read the 
#>   contents of `path` and pass the contents to `srcdoc`. 
#> <!DOCTYPE html>
#> <html lang="en">
#> <head>
#> <meta charset="utf-8"/>
#> <style>body{background-color:white;}</style>
#> 
#> 
#> </head>
#> <body>
#> <p>Test</p>
#> </body>
#> </html>
CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

gadenbuie commented 1 year ago

Thanks for the input, I'm going to move this to draft while I consider next steps.

gadenbuie commented 1 year ago

@cpsievert @schloerke I updated this PR to reduce the scope and address the immediate problem posed in #124. With this change, includeHTML() will issue a warning if it detects that the user is attempting to include a complete HTML document.

I've opened a new issue #383 to track thoughts and progress toward a new includeHTMLDocument() function, which uses the implementation I've removed from this PR.

wch commented 1 year ago

Looks good -- just needs a NEWS entry.