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

Sketch dryRun() implementation #833

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Sketch dryRun() implementation #833

wants to merge 8 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented May 3, 2023

To fix #725.

Fixes #208. Fixes #253. Fixes #256.

@hadley hadley marked this pull request as ready for review May 8, 2023 16:00
@hadley hadley requested review from aronatkins and kevinushey and removed request for aronatkins May 8, 2023 16:00
@hadley
Copy link
Member Author

hadley commented May 8, 2023

@aronatkins @kevinushey I still need to think up a testing strategy, but I think the overall strategy is feeling pretty solid and it'd be useful to get your thoughts.

@hadley hadley requested a review from aronatkins May 8, 2023 16:42
#' @description
#' `r lifecycle::badge("experimental")`
#'
#' `dryRun()` runs your app locally, attempting to simulate what will happen
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/app/content/

appRunner <- function(appMode) {
switch(appMode,
"rmd-static" = ,
"rmd-shiny" = function(primaryDoc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rmd-shiny should use rmarkdown::run(file = NULL, dir = getwd())

Copy link
Contributor

Choose a reason for hiding this comment

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

When the R Markdown content is a site: rmarkdown::render_site(envir = new.env())

Copy link
Member Author

Choose a reason for hiding this comment

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

I already set the working directory above, but I'll take the other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ::run(..) signature is necessary to have R Markdown do its "run the whole directory" magic.. it's not setting the working directory. I think that's this logic: https://github.com/rstudio/rmarkdown/blob/main/R/shiny.R#L83-L107

Copy link
Member Author

Choose a reason for hiding this comment

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

I just meant the dir argument

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Once you give file = NULL, the dir value is necessary, as its default value of dirname(file) is meaningless.

rmarkdown::render(primaryDoc, quiet = TRUE)
},
"quarto-static" = ,
"quarto-shiny" = function(primaryDoc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

quarto-shiny should:

  1. render with Quarto
  2. use rmarkdown::run(file = NULL, dir = getwd())

"api" = function(primaryDoc) {
plumber::pr_run(plumber::pr("plumber.R"))
},
cli::cli_abort("Content type {appMode} not currently supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Rendered content and "static" -- we could start an HTML server against the bundleDir so they could see exactly what files they are about to deploy. Maybe that would help cases where folks forget to include some images/CSS.

The result of this would be that every content type starts some type of HTTP server that they would interact with for validation.

#' Both of these will work locally but fail on the server. [lint()]
#' uses an alternative technique (static analysis) to detect many of these
#' cases.
#'
Copy link

@andpatt andpatt May 10, 2023

Choose a reason for hiding this comment

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

Another limitation is that:

  • dryRun() doesn't help if the server does not have required system dependencies that are present
    on the machine on which you're running this dryRun().

Maybe a bit nuanced to mention?

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