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

Automatically load 'helpers' in R/ directory at runtime #2547

Merged
merged 24 commits into from Aug 28, 2019

Conversation

@trestletech
Copy link
Contributor

commented Aug 8, 2019

Sources files ending in .r (case-insensitive) found in the R/ directory (case-sensitive) when starting the app. If using app.R, the helpers are sourced before app.R. If using server.R, the global.R file is source first, then the helpers, then your server.R and ui.R files (when needed).

This requires opting in via setting the shiny.autoload.r option to TRUE. Without setting this option, this PR changes nothing.

Environments

When shiny.autoload.r is TRUE, the environments are set as follows:

With server.R and ui.R:

+ global env (global.R)
|
+-+ helpers env (R/ files)
| |
| +-- ui.R env
| |
| +-- server.R env

The only change here is the introduction of a new environment in between the child ui and server environments into which the helpers are loaded.

With app.R

+ global env
|
+-+ helpers env (R/ files)
| |
| +-- app.R

Testing Notes

There's pretty good automated unit tests included in this PR to confirm the behavior around environment organization and when the appropriate code is run. So hopefully the manual QA burden here is mostly just doing a high-level smoke test to confirm that the new behavior is reasonable.

Test the new functionality

  1. Install the latest Shiny from master: `remotes::install_github("rstudio/shiny")
  2. Restart R
  3. Try running the shiny example 168. I have this app in shiny-examples here.You'll see that it fails with an error like:
ERROR: could not find function "counterButton"
  1. Run the following R code to enable autoloading:
options(shiny.autoload.r = TRUE)
  1. Rerun example app 168 and confirm that the app now runs. You should see a button that you can click on to increment a counter. If that works, that demonstrates that the R file in the R/ directory was automatically loaded.
  2. If you want to do more experimentation, you can disable autoloading using the following command:
options(shiny.autoload.r = FALSE)

Confirm old functionality

  • Run an app that requires global.R (such as this one) to confirm that it still works.
trestletech added 10 commits Aug 5, 2019
Load helpers in R/ on app startup
No support yet for case-sensitive file systems when loading the dir.
Load helpers into isolated environment
And scaffold out the tests.
Add news
and other minor changes from self-review

@trestletech trestletech requested a review from wch Aug 8, 2019

R/app.R Outdated Show resolved Hide resolved
R/app.R Outdated Show resolved Hide resolved
trestletech added 4 commits Aug 12, 2019
Only load top-level R files in R/
Ignores nested directories to better follow R package conventions. We want to align well so that this structure is portable to golem.
@hadley

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

I wonder if there should be some way to force the path to R/ to be related to the project? Because if you imagine this is as step to turning an app into a standalone package, it would be weird to leave the app.R in the root directory of the package. I'm not sure how you would toggle this mode though? Maybe the presence of a DESCRIPTION file? Maybe if the app file is in a specially named directory? (e.g. Golem used to put it in inst/app)

@hadley

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Or maybe the model should be that if your app (or apps?) are in a package they're exposed by a specially named function so that you'd just do devtools::load_all() then launchMyApp()? This is the current golem approach (although with a different naming scheme). I think it's worth spitballing this a little.

@wch
wch approved these changes Aug 27, 2019
Copy link
Collaborator

left a comment

Looks good - just a couple questions. We should hold off merging into master until after the upcoming release. Another option is to merge to a separate branch.

R/app.R Outdated Show resolved Hide resolved
R/app.R Show resolved Hide resolved
trestletech added 3 commits Aug 27, 2019
R/app.R Outdated Show resolved Hide resolved
R/app.R Show resolved Hide resolved
R/app.R Outdated Show resolved Hide resolved
R/app.R Outdated Show resolved Hide resolved
trestletech added 2 commits Aug 28, 2019
- update NEWS
- only source global in server.R mode
- only use intermediary environment if opted-in to autoloading.
@wch
wch approved these changes Aug 28, 2019
NEWS.md Outdated
* Resolved [#1433](https://github.com/rstudio/shiny/issues/1433): `plotOutput()`'s coordmap info now includes discrete axis limits for **ggplot2** plots. As a result, any **shinytest** tests that contain **ggplot2** plots with discrete axes (that were recorded before this change) will now report differences that can safely be updated. This new coordmap info was added to correctly infer what data points are within an input brush and/or near input click/hover in scenarios where a non-trivial discrete axis scale is involved (e.g., whenever `scale_[x/y]_discrete(limits = ...)` and/or free scales across multiple discrete axes are used). ([#2410](https://github.com/rstudio/shiny/pull/2410))

### Improvements

* Resolved [#2402](https://github.com/rstudio/shiny/issues/2402): An informative warning is now thrown for mis-specified (date) strings in `dateInput()`, `updateDateInput()`, `dateRangeInput()`, and `updateDateRangeInput()`. ([#2403](https://github.com/rstudio/shiny/pull/2403))

* If the `shiny.autload.r` option is set to `TRUE`, all files ending in `.r` or `.R` contained in a directory named `R/` adjacent to your application are sourced when your app is started. This will become the default Shiny behavior in a future release. ([#2547](https://github.com/rstudio/shiny/pull/2547))

This comment has been minimized.

Copy link
@wch

wch Aug 28, 2019

Collaborator

typo: shiny.autload.r

@trestletech trestletech merged commit 99ac85f into master Aug 28, 2019

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@trestletech trestletech deleted the jeff/feature/helpers branch Aug 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.