Prerender shiny rmd in separate environment

[gadenbuie]

When running an rmarkdown document with runtime: shinyrmd or shiny_prerendered, if the document has not yet been pre-rendered or if the pre-render is out of date, rmarkdown::run() will first call render() on the document.

Because the document rendering happens in the same environment as the subsequent run() step, code chunks that should be pre-render only are leaked into the shiny app, for the first run only. On subsequent runs of the same document, objects created by normal R chunks will not be available to the shiny app.

This is particularly important for learnr, where we expect that code and functions written in standard R chunks are not made available to exercises — in other words the results of static R chunks should not be accessible from the running shiny app.

In the following example, we expect that submitting local in the exercise should always return base::local, but on the first rmarkdown::run() call it will be masked by the local defined in the presumed static R chunk local-only.

---
title: "Tutorial"
output: learnr::tutorial
runtime: shiny_prerendered
---

```{r setup, include=FALSE}
library(learnr)
knitr::opts_chunk$set(echo = FALSE)
global <- "hello, global"
```

## Topic 1

### Exercise 

```{r local-only}
local <- "hello, local"
```

```{r two-plus-two, exercise=TRUE}
local
```

This PR addresses the above problem by using a separate environment for rendering the shinyrmd document. To avoid surprises, the pre-render environment is a child of the requested rendering environment EDIT: unless that environment is NULL, in which case we use a child of the global env.

[yihui]

Looks good to me. Please feel free to request review from people with more expertise. Thanks!

[cderv]

This looks ok to me too.

It seems what you need is to run a tutorial using

rmarkdown::run("lessons.Rmd", envir = new.env())

and we are internalizing this for any shiny prerendered document, and not just learnr, right ?

I think it is safe to render the Rmd in a new environment with parent. I am just surprised that this happens, but I guess it it because learnr is using an environment with GlobalEnv as parent, and indeed render() will be default populate the global env. I think it is a good idea it does not for this type of doc.

Just to understand correctly: in which case did you encounter the need for render_args$envir to be NULL ?
rmarkdown::run() will set it to parent.frame(), so I don't think there is a case where it will be NULL. So I am curious.

I don't think I am the one with the most expertise on this. I mostly did reverse engineering on all this last year.

@yihui, should we ask JJ or Joe who know better shiny_prerender from the start ? (is there anyone else ?)

But I think this change is quite small and safe, so it looks ok to merge. I believe learnr is the main user of shiny prerendered document anyway, so if it is good for you and your team @gadenbuie, I think it is fine.

We should add a NEWS bullet maybe as this is a change that may be impactful if anyone was using this somehow (but I don't think so).

[gadenbuie]

It seems what you need is to run a tutorial using r rmarkdown::run("lessons.Rmd", envir = new.env()) and we are internalizing this for any shiny prerendered document, and not just learnr, right?

Yes, to an extent. Calling run() with an environment wouldn't resolve the problem though because the render step will still be evaluated in that environment, which leaks the result of static code into the shiny app for the first run. This means that the issue also affects shiny prerendered Rmd apps that are run via the Run Document button in RStudio. I'm sure learnr is one of the few places where this matters, but I think it will reduce gotchas for all shiny + rmarkdown users.

Just to understand correctly: in which case did you encounter the need for render_args$envir to be NULL ?

I encountered the NULL render_args$envir in the tests, otherwise it does appear that this argument will be filled in by the user facing functions.

rmarkdown/tests/testthat/test-shiny_prerendered.R

Line 29 in 5952bb6

html <- shiny_prerendered_html(tmp_rmd, list(quiet = TRUE))

[gadenbuie]

Apologies for the force-push, I realized this branch was based on the now defunct master branch. I rebased it on main and added a NEWs entry.

[cderv]

Yes, to an extent. Calling run() with an environment wouldn't resolve the problem though because the render step will still be evaluated in that environment, which leaks the result of static code into the shiny app for the first run.

Oh I understand correctly now. Thanks for the clarification.