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

htmlDependency used in dynamic UI breaks when script is provided as a named list instead of a string #3345

Closed
daattali opened this issue Mar 24, 2021 · 4 comments · Fixed by #3395
Milestone

Comments

@daattali
Copy link
Contributor

Not sure if this should be filed under {htmltools} or {shiny} - I filed it here because the interaction with shiny is what's broken from what I can tell.

To use htmlDependency to load a javascript module, we need to use script = list(src = "test.js", type = "module"). This works fine when the dependency is resolved from the UI, but it seems to parse incorrectly when done dynamically. Example:

library(shiny)

ui <- fluidPage(
  htmltools::attachDependencies(
    "test",
    htmltools::htmlDependency(
      name = "test",
      version = "1.0.0",
      src = c(href = "https://cdn.jsdelivr.net/gh/daattali/htmldependencybug"),
      script = list(src = "test.js", type = "module")
    )
  )
)

server <- function(input, output, session) {
}

shinyApp(ui, server)

The above code works - you'll see a javascript alert.

But if the content is loaded dynamically -- either using insertUI or using a uiOutput/renderUI -- then it breaks. Example:

library(shiny)

ui <- fluidPage(
  uiOutput("test")
)

server <- function(input, output, session) {
  output$test <- renderUI({
    htmltools::attachDependencies(
      "test",
      htmltools::htmlDependency(
        name = "test",
        version = "1.0.0",
        src = c(href = "https://cdn.jsdelivr.net/gh/daattali/htmldependencybug"),
        script = list(src = "test.js", type = "module")
      )
    )
  })
}

shinyApp(ui, server)

The page loads but the javascript module isn't loaded. Looking at the page source, I can see that it's trying to load a script with src of https://cdn.jsdelivr.net/gh/daattali/htmldependencybug/%5Bobject%20Object%5D - this is what's causing this bug. It seems that shiny does not know how to deal with the script argument being a named list, even though a named list is allowed according to the documentation.

@daattali
Copy link
Contributor Author

This is not a problem only for modules, this seems to be an issue whenever script is given a named list instead of a string. Even if I only provide the source, but I do so using list(src = ...) instead of using a string, the same problem happens. Example:

library(shiny)

ui <- fluidPage(
  uiOutput("test")
)

server <- function(input, output, session) {
  output$test <- renderUI({
    htmltools::attachDependencies(
      "test",
      htmltools::htmlDependency(
        name = "test",
        version = "1.0.0",
        src = c(href = "https://cdn.jsdelivr.net/gh/daattali/htmldependencybug"),
        script = list(src = "test_simple.js")
      )
    )
  })
}

shinyApp(ui, server)

@daattali daattali changed the title Unable to load a JavaScript module with htmlDependency after page load - because shiny fails to correctly parse script=list(...) htmlDependency used in dynamic UI breaks when script is provided as a named list instead of a string Mar 27, 2021
@daattali
Copy link
Contributor Author

@schloerke @wch my original title and description were much more complicated than needed. I've simplified the problem, so hopefully it's easier to understand now and will get looked at :)

@schloerke
Copy link
Collaborator

Thank you for the great reprexes!!


When put in the UI, it is eventually rendered using htmltools::renderDependencies(deps). https://github.com/rstudio/htmltools/blob/9abc3f2d9e23e9f02a0945c526d8a3de77f9cec4/R/html_dependency.R#L459 from within R.

In dynamic renderUI(), the script tag is rendered using JS via

if (dep.script && !restyle) {
var scripts = $.map(asArray(dep.script), function(scriptName) {
return $("<script>").attr("src", href + "/" + encodeURI(scriptName));
});
$head.append(scripts);
}
from the v1.6.0 release. This bug has been this way for [>= 6 yrs](https://github.com/rstudio/shiny/blame/b57cb6c8e1ed2e8d75ea0ba47854b15e03bc1287/srcjs/output_binding_html.js#L182-L187 (So not a bug introduced with the TypeScript conversion on master.)

When this gets fixed, it either needs to leverage htmltools (my preference), or match the logic of htmltools in the browser code. Matching the logic of htmltools will probably be the actual solution as objects can not be fully converted to tags before being sent to the browser. If they could, then there would be no need to define JS output types within the browser.


This app provides a work around using the htmltools approach...

library(shiny)

ui <- fluidPage(
  uiOutput("test")
)

server <- function(input, output, session) {
  output$test <- renderUI({

    ret <- htmltools::attachDependencies(
      "test",
      htmltools::htmlDependency(
        name = "test",
        version = "1.0.0",
        src = c(href = "https://cdn.jsdelivr.net/gh/daattali/htmldependencybug"),
        script = list(src = "test.js", type = "module")
      )
    )

    # ... reformat into tags ...

    deps <- htmltools::htmlDependencies(ret)

    ret_no_deps <- ret
    htmltools::htmlDependencies(ret_no_deps) <- NULL

    htmltools::tagList(
      htmltools::tags$head(
        htmltools::renderDependencies(deps)
      ),
      ret_no_deps
    )
  })
}

shinyApp(ui, server)

@schloerke schloerke added this to the 1.7 milestone Mar 30, 2021
@daattali
Copy link
Contributor Author

Thanks for finding out the culprit @schloerke

Your workaround does work for the simple case I mentioned here, but it does not work for my real usecase. I may be wrong, but it might be I'm using local files rather than www URLs, and I haven't explicitly used addResourcePath(). I imagine that rendering using attachDependencies automatically adds the relevant path as a resource, whereas renderDependencies only provides html markup. This analysis may be wrong.

In either case, I see you've added this to the next release milestone, so I prefer to wait for the fix rather than a hacky workaround

cpsievert added a commit that referenced this issue May 20, 2021
* Close #3345: correctly render script tags defined as list() objects

* implement boolean attrs; use vanilla JS

* Update news

* avoid toggleAttribute

* yarn lint (GitHub Actions)

* code review

Co-authored-by: cpsievert <[email protected]>
cpsievert added a commit to rstudio/shinycoreci-apps that referenced this issue May 20, 2021
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 a pull request may close this issue.

2 participants