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

feat: Use stable leaflet.providers html dep if available #884

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Oct 5, 2023

Fixes #732
Fixes #843
Closes #803
For rstudio/leaflet.providers#34

This PR pairs with rstudio/leaflet.providers#36 to use the html dependency created by leaflet.providers, if available.

With the latest version of leaflet.providers, leaflet will use a stable html dependency that will not change between R sessions and will, in general, be consistent in the knitr cache (excepting package updates which should break the cache anyway).

In the presence of an older version of leaflet.providers, leaflet will still create a temporary html dependency, so this is a backwards-compatible change.

With rstudio/leaflet.providers#36 loaded, you'll see:

leaflet()  |>
  setView(lng = -71.0589, lat = 42.3601, zoom = 12) |> 
  addTiles() |> 
  addProviderTiles("Stamen.Toner") |> 
  htmltools::findDependencies() |>
  purrr::keep(\(x) x$name == "leaflet-providers")
#> [[1]]
#> List of 10
#>  $ name      : chr "leaflet-providers"
#>  $ version   : chr "2.0.0"
#>  $ src       :List of 1
#>   ..$ file: chr "/Users/garrick/work/rstudio/leaflet.providers/inst"
#>  $ meta      : NULL
#>  $ script    : chr "leaflet-providers_2.0.0.js"
#>  $ stylesheet: NULL
#>  $ head      : NULL
#>  $ attachment: NULL
#>  $ package   : NULL
#>  $ all_files : logi FALSE
#>  - attr(*, "class")= chr "html_dependency"

And for versions other than the bundled leaflet-providers:

use_providers(get_providers("1.13.0"))

leaflet()  |>
  setView(lng = -71.0589, lat = 42.3601, zoom = 12) |> 
  addTiles() |> 
  addProviderTiles("Stamen.Toner") |> 
  htmltools::findDependencies() |>
  purrr::keep(\(x) x$name == "leaflet-providers")
#> [[1]]
#> List of 10
#>  $ name      : chr "leaflet-providers"
#>  $ version   : chr "1.13.0"
#>  $ src       :List of 1
#>   ..$ href: chr "https://unpkg.com/[email protected]"
#>  $ meta      : NULL
#>  $ script    : chr "leaflet-providers.js"
#>  $ stylesheet: NULL
#>  $ head      : NULL
#>  $ attachment: NULL
#>  $ package   : NULL
#>  $ all_files : logi FALSE
#>  - attr(*, "class")= chr "html_dependency"

@jaredlander
Copy link

Looks like this is a better version of #803. I was just coming across this problem yet again, so I'm glad it's being fixed.

@gadenbuie
Copy link
Member Author

@jaredlander Ah sorry, I didn't realize there was prior work on this, I kinda just dropped in without a ton of context. Thanks for your previous PR and the interim fix! Barret and I did discuss doing something similar to what you proposed (picking a stable place to put the dependency file), but I think this approach will end up being a little easier to work with in the long run. End users won't have to think about any intermediate files and knitr/quarto caching should just work🤞.

If you're running into this now and have the ability to test this PR with rstudio/leaflet.providers#36, I'd love to hear if that works for you. You can install from both PRs with something like this:

remote::install_github(c("rstudio/leaflet#884", "rstudio/leaflet.providers#36"))

@jaredlander
Copy link

@gadenbuie I'm just happy for whatever makes it work.

I'll try the PR versions soon hopefully. Right now I'm fighting an uphill battle trying to use {webshot} to take screenshots of maps made with {leafgl} (it won't: wch/webshot#45). And no, {webshot2} isn't ideal because I'm running this on a server and want to avoid installing Chrome.

@gadenbuie
Copy link
Member Author

@schloerke The failure here is related to terra's use of inline functions (\(x)), which should be fixed in the next version of terra (I submitted a PR back on 9/7). Anyway, we can move forward here.

Copy link
Contributor

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@schloerke schloerke merged commit bc7b4d2 into main Oct 13, 2023
8 of 10 checks passed
@schloerke schloerke deleted the feat/stable-leaflet-providers-dep branch October 13, 2023 14:13
@schloerke
Copy link
Contributor

@jaredlander

And no, {webshot2} isn't ideal because I'm running this on a server and want to avoid installing Chrome.

If you any browser that will follow the chrome devtools protocol, you may use it. Example of using Brave browser for the default {chromote} browser: rstudio/shinytest2#330 (comment)

I hope you find a path forward with your WebGL screenshots.

@jaredlander
Copy link

If there was a completely headless server (with no GUI), I'd be more inclined to install that. Do you know any?

jbdorey added a commit to jbdorey/BeeBDC that referenced this pull request Mar 4, 2024
Fixed an issue caused by a stability fix from leaflet [#884](rstudio/leaflet#884) where the tonerLite base map did not work and so would stop points from showing on the map.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants