Skip to content

createWebDependency can't handle relative htmlDependencies #2383

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

Open
cpsievert opened this issue Apr 11, 2019 · 3 comments · May be fixed by #2384
Open

createWebDependency can't handle relative htmlDependencies #2383

cpsievert opened this issue Apr 11, 2019 · 3 comments · May be fixed by #2384

Comments

@cpsievert
Copy link
Collaborator

cpsievert commented Apr 11, 2019

Minimal example, this works as expected

library(htmltools)
dep <- htmlDependency(
  name = "jquery",
  version = "1.0.0",
  src = c(file = "www/shared"),
  script = "jquery.min.js",
  package = "shiny"
)
renderDependencies(list(dep))
<script src="www/shared/jquery.min.js"></script>

This does not:

shiny::createWebDependency(dep)
Error in value[[3L]](cond) : 
  Couldn't normalize path in `addResourcePath`, with arguments: `prefix` = 'jquery-1.0.0'; `directoryPath` = 'www/shared'

I noticed this in a shiny app that uses crosstalk and leaflet. The dev version of crosstalk::crosstalkLibs() now returns html dependencies with relative paths which ends up being a problem for this line of leaflet

https://github.com/rstudio/leaflet/blob/44df7d18a2618e0aeecb4145d765e597ec65878b/R/utils.R#L201

Since others might want to use shiny::createWebDependency() in a similar way, I figured the fix should derive from here.

@jcheng5
Copy link
Member

jcheng5 commented Apr 12, 2019

It looks like when Shiny internally calls createWebDependency, it (first calls htmltools::resolveDependencies)[https://github.com/rstudio/shiny/blob/master/R/html-deps.R#L46-L49], which turns the relative paths into absolute ones. That's why these dependencies work with Shiny, but not with leaflet. I think it's actually incorrect for leaflet (or anyone) to pass unresolved dependencies to createWebDependency; it means, for one thing, there could be multiple copies of i.e. name="bootstrap" and the last one would win (rather than the one with the highest version number).

That's a real sucky API though...

@jcheng5
Copy link
Member

jcheng5 commented Apr 12, 2019

Maybe resolveDependencies should add a class to each dependency object, and createWebDependency should warn if the objects it gets aren't marked with the class?

@cpsievert
Copy link
Collaborator Author

cpsievert commented May 2, 2019

Maybe resolveDependencies should add a class to each dependency object, and createWebDependency should warn if the objects it gets aren't marked with the class?

This doesn't seem quite right either. You could still end up in a situation where you have manually combined 2 (or more) lists of the same resolved dependencies, which should "un-resolve" them, but there's no way for us to detect or prevent that from happening. If we're really concerned about duplicated dependencies, maybe createWebDependency() could look for likely suspects in .globals$resources?

pattern <- paste0("^", dependency$name, "-")
if (grepl(pattern, names(.globals$resources))) {
  warning("HTML dependency ", dependency$name, " has likely already been registered as a web resource.")
}

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