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: Add support for running Shiny for R apps #30

Merged
merged 22 commits into from
Apr 22, 2024

Conversation

gadenbuie
Copy link
Collaborator

@gadenbuie gadenbuie commented Apr 2, 2024

Closes #29

  • Clones shiny.python.runApp into a new shiny.r.runApp command.
  • Does not include app debugging with shiny.r.debugApp (yet)
  • Does not support codespaces
  • Adds Shiny for R devmode and port options
  • Renames the configuration section "Shiny"
  • Calls shiny::devmode() before running the app when shiny.r.devmode is true or options(shiny.autoreload = TRUE) when false (to maintain the reload feature).

Todo

  • Should Shiny for R and Shiny for Python apps use the same Shiny terminal?
    • Yes, since they'd use the same Simple browser window as well.
  • Auto-reload port - is this required in R?
    • I don't think so. The extension either calls shiny::devmode() or options(shiny.autoreload = TRUE) before running the app and both appear to be working as expected.
  • Separate setting for port for Shiny for R apps?
    • Yes, otherwise it's confusing that the Shiny > Python > Port option is for both.
    • I looked into changing the setting to shiny.port but I couldn't find a good mechanism for migrating users from shiny.python.port to shiny.port.
  • Add setting for enabling devmode in R, defaults to true
  • Call Rscript on Mac/Linux or Rscript.exe if on Windows
  • How to get the path to Rscript.exe on Windows?

@gadenbuie gadenbuie marked this pull request as ready for review April 3, 2024 14:32
@gadenbuie gadenbuie requested a review from jcheng5 April 3, 2024 14:32
package.json Show resolved Hide resolved
@jcheng5
Copy link
Collaborator

jcheng5 commented Apr 3, 2024

In package.json we take a dependency on the ms-python.python package. Maybe we should make that optional and testing for its existence when actually running a Shiny for Python app, as it would be a shame if R users got that extension dragged along just by installing the extension.

src/run-r.ts Outdated
? "shiny::devmode()"
: "options(shiny.autoreload = TRUE)";

const runApp = `${devOrReload}; shiny::runApp("${path}", port=${port}, launch.browser=FALSE)`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

${path} needs escaping here. Not only for edge cases but on Windows the path will contain backslashes (I assume).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC escapeCommandForTerminal() handles this, It's pretty aggressive and has worked in my testing. In fact, the R code in runApp isn't inside quotes because it's handled correctly by escapeCommandForTerminal() (on my system at least, I may be missing something about how that function works).

Copy link
Collaborator Author

@gadenbuie gadenbuie Apr 4, 2024

Choose a reason for hiding this comment

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

There are actually two issues happening here:

  1. I've just used Rscript as the command which likely won't work on Windows, where we might need the full path to the Rscript binary, e.g. "C:\Program Files\R\R-3.6.0\bin\Rscript.exe".

  2. Typically, I'd use Rscript -e 'shiny::runApp("app.R")' in my terminal, but when I used this formulation escapeCommandForTerminal() broke the command by escaping " within the '' quotes.

Given the above, I've refactored the rRunApp() command to use a packaged runShinyApp.R script that takes commandline arguments for path, port and --devmode.

That means that the escaping now works as expected, but leaves us with the problem of figuring out the full path to Rscript.exe on Windows. Do you have any ideas about how to approach that problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might not be as bad as I was thinking. I paired with @andrie this morning, who uses rig to install R on Windows, and in Powershell 7 and cmd.exe Rscript can be called directly. I'm pretty sure this is because rig adds .bat files to create an alias to run Rscript.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you refactor to use runShinyApp.R only because of the escapeCommandForTerminal() behavior? We can definitely fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can count on rig, this is my first time even hearing of it. You can see how the R extension does it here (and RStudio does something similar):

https://github.com/REditorSupport/vscode-R/blob/d582b9184838a32352f624e451d4c1856f347b9c/src/util.ts#L98
https://github.com/REditorSupport/vscode-R/blob/d582b9184838a32352f624e451d4c1856f347b9c/src/util.ts#L63

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcheng5 I've implemented this using the vscode-R extension as inspiration. I also tested locally on a Windows VM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you refactor to use runShinyApp.R only because of the escapeCommandForTerminal() behavior? We can definitely fix that.

It might have started out this way, but we also run shiny::devmode() or set options(shiny.autoreload) before running the app. With that and the port-setting logic, I found it a lot easier to use a separate R script than to try to build the R code in strings.

I did add some logic to echo the shiny::runApp() command so users could copy and paste the command into an R console.

src/run-r.ts Outdated Show resolved Hide resolved
@gadenbuie
Copy link
Collaborator Author

In package.json we take a dependency on the ms-python.python package. Maybe we should make that optional and testing for its existence when actually running a Shiny for Python app, as it would be a shame if R users got that extension dragged along just by installing the extension.

Are these two entries in package.json jointly declaring the dependency on the Python Extension?

  "extensionDependencies": [
    "ms-python.python"
  ],
  "dependencies": {
    "@vscode/python-extension": "^1.0.5"
  }

I generally agree with you, but I don't think we need to do that work yet. I think it'd be okay for us to require the Python extension for now (it's a very popular extension and I'd imagine the number of R users who are using VSCode but not that extension is currently rather small).

@jcheng5
Copy link
Collaborator

jcheng5 commented Apr 4, 2024

extensionDependencies is what makes VS Code install the Python extension. dependencies is bringing an npm package into our own project--in this case, it's JavaScript stubs for calling into the Python extension.

@jcheng5
Copy link
Collaborator

jcheng5 commented Apr 4, 2024

I implemented making the Python extension a soft dependency, but I don't feel that strongly about it either way.

rscripts/runShinyApp.R Outdated Show resolved Hide resolved
Co-authored-by: Joe Cheng <joe@posit.co>
@jcheng5 jcheng5 self-requested a review April 22, 2024 16:57
@gadenbuie gadenbuie merged commit 505043e into posit-dev:main Apr 22, 2024
1 check passed
@gadenbuie gadenbuie deleted the shiny-for-r branch April 22, 2024 20:14
@gadenbuie gadenbuie mentioned this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add shiny.r.runApp command
2 participants