ThinkR-open / golem

A Framework for Building Robust Shiny Apps
https://thinkr-open.github.io/golem/
Other
902 stars 132 forks source link

Golem apps should not have to import {golem} #597

Closed antoine-sachet closed 1 year ago

antoine-sachet commented 3 years ago

The {golem} package is rather beefy, so I'd rather avoid having it as a dependency. For a number of reasons: it makes the docker image bigger, it make the build time longer, it increases the attack surface for dependency injection, etc.

Look at everything it pulls that you never need for an app (snippet from docker build):

Downloading GitHub repo ThinkR-open/golem@558a715bfd601468cd7131a719014e3a256d9390
Installing 35 packages: ini, gitcreds, zip, rstudioapi, credentials, rematch2, diffobj, prettyunits, callr, rprojroot, pkgbuild, desc, xfun, stringr, markdown, highr, evaluate, whisker, rappdirs, gh, gert, fs, waldo, praise, pkgload, brio, xml2, knitr, brew, attempt, usethis, testthat, roxygen2, here, dockerfiler

As far as I can tell, the only reason an app has to import golem is because of this function in app_ui.R:

#' Add external Resources to the Application
#'
#' This function is internally used to add external
#' resources inside the Shiny application.
#'
#' @import shiny
#' @importFrom golem add_resource_path activate_js favicon bundle_resources
#' @noRd
golem_add_external_resources <- function(){

  add_resource_path(
    'www', app_sys('app/www')
  )

  tags$head(
    favicon(ext = "png"),
    bundle_resources(
      path = app_sys('app/www'),
      app_title = get_golem_config("document")$browser_title
    ),
 # ...
}

I would suggest moving add_resource_path, activate_js, favicon, bundle_resources out of golem.

These are all really short helper functions which could simply live in a local R file in, like currently with golem_utils_server.R and golem_utils_ui.R.

ColinFay commented 3 years ago

Hey,

You're right and this will be the aim of {prague} (related: #270).

This is not a trivial integration so I'm planning on doing this migration once {golem} 0.3.0 is on CRAN :)

Colin

antoine-sachet commented 3 years ago

Great minds :) Apologies, I did do a quick search of the issues but did not spot it was already on the radar.

I've done it manually in the past - it only takes a minute for me because I didn't need any of the golem boilerplate so I removed most of it.

For example, I just remove with_golem_options from run_app (I am curious to know why it is needed btw), and bundle_resources (also not sure why it is needed as js and css in www/ seem to work fine without it). activate_js is imported but not used so that's gone as well.

That only leaves favicon and add_resource_path which I just copy in golem_utils_ui.R.

ColinFay commented 3 years ago

Yeah the full story is that we've been thinking about this for a while now, but as that's not a trivial change (I mean at the {golem} level) I've been delaying this.

with_golem_options() allows to smoothly pass args to the run_app() functions. You can get more info about that here: https://engineering-shiny.org/golem.html#run_app.r

bundle_resources() bundles the resources from app/www (css and js), and add a head to the app. It should work if you manually link them in your app. Something you should be careful is that the reason it might be working without it is that your browser has cached the css, js and favicon from a previous visit to the application.

activate_js() is used inside bundle_resources() (but used to be straight inside app_ui()) so yeah, probably it needs to go as I forgot to remove it :)

ColinFay commented 3 years ago

Ok so I can't stop going back and forth with this 🤔

I feel like having another package just for the deployment might add complexity for both {golem} developers and application devs.

The necessary packages for a golem app to work in prod are :

Possibly {pkgload} if you deploy on Connect and stuff.

As far as I see it, all the other packages could be Suggests inside {golem}, with a check and a suggestion to install whenever needed.

For example, add_dockerfile() could do something like :

if (! requireNamespace("dockerfiler") ){
  stop("`{dockerfiler}` is required, please install it first (for example with `install.packages('dockerfiler')`")
}

This is for example done in purrr::map2_df

> purrr::map2_df
function (.x, .y, .f, ..., .id = NULL) 
{
    if (!is_installed("dplyr")) {
        abort("`map2_dfr()` requires dplyr")
    }
    .f <- as_mapper(.f, ...)
    res <- map2(.x, .y, .f, ...)
    dplyr::bind_rows(res, .id = .id)
}

My wild guess is that in most cases it would be transparent for the dev for the following reasons:

  1. most packages are already installed on the dev machine :
  1. For packages used in prefilled Rmd, RStudio will suggest to install them if they are missing

  2. Some packages are only needed in very specific cases

So I feel we could switch all these as Suggests, just like what {purrr} does with {dplyr}: https://github.com/tidyverse/purrr/blob/master/DESCRIPTION#L33. Then all these Suggests won't be installed in the prod environment.

Any thoughts?

ColinFay commented 2 years ago

Will be working on this soon

ColinFay commented 2 years ago

Functions needed for deployment seem to be:

ColinFay commented 2 years ago

Here is the current state of affair (version 0.3.2):

ColinFay commented 1 year ago

update : almost there, still have to rework desc, rstudioapi, and here

VincentGuyader commented 1 year ago

update : the actual dev branch just get a slimming cure.

the number of Import is very low (and quite close to shiny itself)

ColinFay commented 1 year ago

We have a release candidate for the new version of golem which has the following dependencies:

Imports: 
    attempt (>= 0.3.0),
    config,
    here,
    htmltools,
    rlang (>= 1.0.0),
    shiny (>= 1.5.0),
    utils,
    yaml

This creates the following dependency trees:

> nrow(pak::local_deps())
[1] 37
> pak::local_deps_tree()
local::. 0.4.0 ✨👷🏽‍♂️ ⬇ (unknown size)
├─attempt 0.3.1 ✨ ⬇ (unknown size)
│ └─rlang 1.0.6 ✨ ⬇ (unknown size)
├─config 0.3.1 ✨ ⬇ (unknown size)
│ └─yaml 2.3.6 ✨ ⬇ (unknown size)
├─here 1.0.1 ✨ ⬇ (unknown size)
│ └─rprojroot 2.0.3 ✨ ⬇ (unknown size)
├─htmltools 0.5.4 ✨ ⬇ (unknown size)
│ ├─digest 0.6.31 ✨ ⬇ (unknown size)
│ ├─base64enc 0.1-3 ✨ ⬇ (unknown size)
│ ├─rlang
│ ├─fastmap 1.1.0 ✨ ⬇ (unknown size)
│ └─ellipsis 0.3.2 ✨ ⬇ (unknown size)
│   └─rlang
├─rlang
├─shiny 1.7.3 ✨ ⬇ (unknown size)
│ ├─httpuv 1.6.6 ✨ ⬇ (unknown size)
│ │ ├─Rcpp 1.0.9 ✨ ⬇ (unknown size)
│ │ ├─R6 2.5.1 ✨ ⬇ (unknown size)
│ │ ├─promises 1.2.0.1 ✨ ⬇ (unknown size)
│ │ │ ├─R6
│ │ │ ├─Rcpp
│ │ │ ├─later 1.3.0 ✨ ⬇ (unknown size)
│ │ │ │ ├─Rcpp
│ │ │ │ └─rlang
│ │ │ ├─rlang
│ │ │ └─magrittr 2.0.3 ✨ ⬇ (unknown size)
│ │ └─later
│ ├─mime 0.12 ✨ ⬇ (unknown size)
│ ├─jsonlite 1.8.4 ✨ ⬇ (unknown size)
│ ├─xtable 1.8-4 ✨ ⬇ (unknown size)
│ ├─fontawesome 0.4.0 ✨ ⬇ (unknown size)
│ │ ├─rlang
│ │ └─htmltools
│ ├─htmltools
│ ├─R6
│ ├─sourcetools 0.1.7 ✨ ⬇ (unknown size)
│ ├─later
│ ├─promises
│ ├─crayon 1.5.2 ✨ ⬇ (unknown size)
│ ├─rlang
│ ├─fastmap
│ ├─withr 2.5.0 ✨ ⬇ (unknown size)
│ ├─commonmark 1.8.1 ✨ ⬇ (unknown size)
│ ├─glue 1.6.2 ✨ ⬇ (unknown size)
│ ├─bslib 0.4.1 ✨ ⬇ (unknown size)
│ │ ├─htmltools
│ │ ├─jsonlite
│ │ ├─sass 0.4.4 ✨ ⬇ (unknown size)
│ │ │ ├─fs 1.5.2 ✨ ⬇ (unknown size)
│ │ │ ├─rlang
│ │ │ ├─htmltools
│ │ │ ├─R6
│ │ │ └─rappdirs 0.3.3 ✨ ⬇ (unknown size)
│ │ ├─jquerylib 0.1.4 ✨ ⬇ (unknown size)
│ │ │ └─htmltools
│ │ ├─rlang
│ │ ├─cachem 1.0.6 ✨ ⬇ (unknown size)
│ │ │ ├─rlang
│ │ │ └─fastmap
│ │ └─memoise 2.0.1 ✨ ⬇ (unknown size)
│ │   ├─rlang
│ │   └─cachem
│ ├─cachem
│ ├─ellipsis
│ └─lifecycle 1.0.3 ✨ ⬇ (unknown size)
│   ├─cli 3.4.1 ✨ ⬇ (unknown size)
│   ├─glue
│   └─rlang
└─yaml

Key:  ✨ new |  ⬇ download | 👷🏽‍♂️ build

As far as I see it for the remaining deps:

VincentGuyader commented 1 year ago

\o/

ColinFay commented 1 year ago

Current version, in a fresh rocker/verse:latest image and just pak installed

> pak::pak("thinkr-open/golem@dev")
✔ Updated metadata database: 2.14 MB in 6 files.                          
✔ Updating metadata database ... done                                     

→ Will install 4 packages.
→ Will download 3 CRAN packages (181.27 kB).
→ Will download 1 package with unknown size.
+ attempt   0.3.1 [bld][dl] (79.26 kB)
+ config    0.3.1 [bld][dl] (69.06 kB)
+ golem     0.4.0 [bld][cmp][dl] (GitHub: 942c998)
+ here      1.0.1 [bld][dl] (32.95 kB)
ℹ Getting 3 pkgs (181.27 kB) and 1 pkg with unknown size
✔ Got here 1.0.1 (source) (52.30 kB)                                            
✔ Got config 0.3.1 (source) (79.95 kB)                  
✔ Got attempt 0.3.1 (source) (106.98 kB)                
✔ Got golem 0.4.0 (source) (987.27 kB)                  
✔ Downloaded 4 packages (1.23 MB)in 16.6s               
ℹ Building attempt 0.3.1
ℹ Building config 0.3.1
ℹ Building here 1.0.1
✔ Built config 0.3.1 (2.1s)                                                     
✔ Built attempt 0.3.1 (2.4s)                                                    
✔ Built here 1.0.1 (2.4s)                         
✔ Installed config 0.3.1  (432ms)                                           
✔ Installed attempt 0.3.1  (352ms)                                          
✔ Installed here 1.0.1  (292ms)                                           
ℹ Packaging golem 0.4.0                                          
✔ Packaged golem 0.4.0 (3s)                                      
ℹ Building golem 0.4.0                                          
✔ Built golem 0.4.0 (15.8s)                                     
✔ Installed golem 0.4.0 (github::thinkr-open/golem@942c998) (186ms)
✔ 1 pkg + 36 deps: kept 33, added 4, dld 4 (NA B) [1m 20.9s]
ColinFay commented 1 year ago

Ok, so it's been a long, long journey but it's now in the latest version of {golem}.

You can check https://golemverse.org/post/golem-0.4.0-release-on-cran/ for more information :)