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

R session watcher #150

Merged
merged 54 commits into from Dec 19, 2019
Merged

R session watcher #150

merged 54 commits into from Dec 19, 2019

Conversation

renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 3, 2019

What problem did you solve?

This PR is an attempt to implement the file-based approach to communicating between vscode-R and a live R session suggested in #143, which also solves #142, #51, and will potentially provide nicer solution to #15, #22, #25, and all other session-related issues (if I'm missing some). Thanks @andycraig for the initial implementation of attaching to active session and showing hover on symbols in globalenv in active session.

The reason I suggest a file-based mechanism is to allow flexible management of R sessions. Under such mechanism, user can easily work with any R sessions, no matter whether it is official R or radian, whether it is started by vscode-R or by user, whether it is in a tmux or screen window, whether there are multiple R sessions, whether it is local or remote. Such loose connection between vscode-R and live R sessions would not impose much constraints on the stability and availability of both sides, because there's no communication between two top-level user inputs.

An R script R/init.R in included in this extension to enable the file-based communication. On extension activation, it would be copy to ~/.vscode-R/init.R. To enable R session watcher, user should add source("~/.vscode-R/init.R") to ~/.Rprofile and then enable r.sessionWatcher experimental option so that the script is executed on R startup.

This PR implements the following features:

  • Attach Active Terminal (by command or clicking status bar item)
  • Auto attach on R session startup: if init.R is sourced in .Rprofile, starting an R session will notify vscode-R to automatically attach to it.
  • Provide hover to global symbol in attached session
  • Show plot file on the fly
  • Show WebView to present htmlwidgets and shiny apps
  • Show WebView for data.frame and list object when calling View()

This PR also enables the following features to be implemented in the future:

  • Provide completion to global symbol in attached session
  • Provide completion to list-like objects (list, environment, S4, R6, etc.) in attached session
  • Plot history navigation
  • WebView history navigation

** Screenshots **

Symbol hover: (first lines provided by latest languageserver, last line provided by vscode-R session watcher)

image

image

Basic plot and ggplot2:

image

image

Show htmlwidgets in WebView:

image

image

Show shiny app:

image

View data.frame:

image

View list:

image

Animation:

image

@nathaneastwood
Copy link

By implementing the web view will this mean we should be able to use tools such as profvis? Might be worth testing.

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 9, 2019

@nathaneastwood, Yes, profvis works. And https://github.com/Ikuyadeu/vscode-R/pull/150/files#diff-2711d8988b4edf6562a0668d7e873c13R32-R34 is the code dedicated to make it work since profvis object uses page_viewer instead of viewer (which is used by most htmlwidgets)

image

If you find any htmlwidget that does not work, please let me know.

@renkun-ken
Copy link
Member Author

@Ikuyadeu @andycraig

I've been using this new set of session-related features in my work for quite a while and find it work well for me. As an experimental feature, when do you think is a good time to roll out for general preview?

I'd like to add some documentation to this feature so that user could easily opt-in. What do you think?

@andycraig
Copy link
Collaborator

andycraig commented Dec 12, 2019

@renkun-ken @Ikuyadeu I've been busy the past couple of weeks and haven't kept up with the developments on this branch. But the last time I used it it was working well and I think it would be fine to roll out as an experimental opt-in feature as soon as the documentation is added. The features in the PR are pretty amazing and I think they will be popular.

@nathaneastwood
Copy link

I'm happy to test it out - but will probably be after Xmas now.

@renkun-ken
Copy link
Member Author

@andycraig Where do you think I should put the documentation of this feature? On README.md or somewhere else?

@andycraig
Copy link
Collaborator

@renkun-ken README.md sounds good to me.

@renkun-ken
Copy link
Member Author

@andycraig I've improved the code and make it use the latest recommended APIs and add a section to README.md. Please take a look and feel free to let me know if there's anything more I should do.

@renkun-ken renkun-ken changed the title WIP: R session watcher R session watcher Dec 13, 2019
@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 14, 2019

Here's a pre-build for those who wants to test first.

vscode-R-1.1.8-session-watcher.vsix.zip

Latest: vscode-R-1.1.8-session-watcher.vsix.zip

@andycraig
Copy link
Collaborator

@renkun-ken Thank you! I will have a look at the new documentation and give it a test this weekend.

@andycraig
Copy link
Collaborator

@renkun-ken I have just tried out the current version of this PR and it works well. I keep saying it but this is a fantastic addition to vscode-R!

I just pushed some changes to make the VSCode tslint TypeScript linter happy, and to update an error message.

I have a couple of questions/requests:

  1. It looks like there is some code from other projects included. As far as I can tell they are all MIT licensed, in which case they are fine to include. I think we are supposed to acknowledge their licenses should be acknowledged by either:

    • Adding their authors to the vscode-R LICENSE file, as it is also MIT, or
    • Copying the projects' MIT license into the top of the code files that come from those projects.

    I THINK these are the correct options but I'm not an expert on licenses so I could be wrong!

  2. I was using the window.showInformationMessage output a lot when I was getting the first steps working, but are the console.log lines still useful? If so, let's leave them there. If not, let's remove them.

@Ikuyadeu I have tried this PR and found no problems. It's great! Once the points above are resolved, I recommend merging it.

@renkun-ken
Copy link
Member Author

For the external js libraries included, I'm also not sure how to deal with the license. Maybe we can take a look at how other extensions deal with it?

The console.log lines are only for debugging purposes. Since the feature is still experimental, I suggest we leave them there as they are not visible to user but developer.

Also note that at the moment init.R uses knitr::kable() to produce HTML to support View(), which make init.R depend on knitr. Let me write a simple function to produce HTML so that init.R only depends on as few packages as possible (only jsonlite).

@andycraig
Copy link
Collaborator

This approach looks pretty good to me: https://opensource.stackexchange.com/a/8654

console.log: Sounds good!

Reducing dependencies: Sounds good!

Thank you!

@vnijs
Copy link

vnijs commented Dec 14, 2019

This PR is fantastic! Any chance it could be made to work with https://github.com/cdr/code-server as well which is (currently) based on VSCode 1.39.2. When I tried to install the vsix file I got the error below.

Unable to install extension 'ikuyadeu.r' as it is not compatible with VS Code '1.39.2'.

@vnijs
Copy link

vnijs commented Dec 14, 2019

FYI Seeing some issues with DT tables

library(DT)
datatable(iris, style = 'bootstrap')

image

DT tables are not shown at all inside a shiny app running in VSCode.

library(radiant.data)
radiant.data::radiant.data()

image

Happy to file a separate issues if needed.

On the "wish-list" would be to allow communication between a shiny app and on open R (Rmd) file ala the functions below from rstudioapi:

rstudioapi::insertText
rstudioapi::getSourceEditorContext

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 15, 2019

@andycraig I refined the approach to View(data.frame or matrix) and now it produces JSON data and no longer depends on knitr.

As for the license, please take any action you think is right. The newly added jsonTable.js is my hand written JS, not sure if there should be a license attached to it.

@renkun-ken
Copy link
Member Author

@vnijs Thanks for your reporting on what you find! For a built against VSCode 1.39.2, I'm not sure how to do that unfortunately, help needed.

As for datatable(iris, style = 'bootstrap'), I can reproduce it too. Does datatable(iris) work for you? Looks like in this case the resources (js, css) are not correctly translated to vscode-resource:// by webview.asWebviewUri(). Let me look into this.

As for the DataTables not showing up in the shiny app, I can reproduce that. Let me look into this.

As for allowing communication between a shiny app and on open R (Rmd) file, I think it would open up a new realm and it does not look straightforward how it should be done but I guess it is possible (https://code.visualstudio.com/api/extension-guides/webview#passing-messages-from-a-webview-to-an-extension). Let's keep a note on this in a separate issue so that we can come back when the basic functionality works smoothly.

I played with radiant.data and it looks amazing, and I also notice that the import data feature does not fully work unless I choose a file in the workspace folder. This is due to the strict limitation of the local resources a WebView can get access to (https://code.visualstudio.com/api/extension-guides/webview#loading-local-content). I'll think about this more later.

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 15, 2019

@vnijs The datatable(iris, style = 'bootstrap') issue seems to be not VSCode-related since it does not work in plain R either.

As for the DataTables not showing up in shiny app, I test with all examples in https://shiny.rstudio.com/articles/datatables.html and everything works well. I guess the problem is radiant.data-specific. I'll take a closer look into it when I have time.

@andycraig
Copy link
Collaborator

@renkun-ken Nice work removing the dependency!

As for the license, please take any action you think is right. The newly added jsonTable.js is my hand written JS, not sure if there should be a license attached to it.

Sure thing, I will push a change later today (hopefully) for the licenses for the third-party code. If jsonTable.js is your original code and you're happy for it to be included under vscode-R's license, then there's no need to attach a license to it.

@renkun-ken
Copy link
Member Author

@andycraig Sounds good! And let jsonTable.js be covered by vscode-R's license.

@Ikuyadeu
Copy link
Member

Ikuyadeu commented Dec 15, 2019

@renkun-ken Thank you for your ton of the works! It is one of the most impactful functions of this extension.

To reducing the project size and managing cost, could you remove the bootstrap and jquery files by updating npm package dependencies?
If this request looks boring, I will push the fixed commit.

@renkun-ken
Copy link
Member Author

@Ikuyadeu Thanks for your notice! It would be very nice to reduce size of the package but I have zero knowledge of how I should work with npm package management in this case. I tried npm install jquery bootstrap but I'm not sure whether it works at runtime since WebViews are created at runtime where HTML includes jquery and bootstrap files on client side.

<script src="${webview.asWebviewUri(Uri.file(path.join(resDir, "js", "jquery.min.js")))}"></script>

Please give me some hint on how I should properly update the npm package dependencies and update the above reference to the JS scripts accordingly?

Also, note that I'm using a Chinese mirror of npm (the official does not work smoothly). All package URIs in package-lock.json are changed with my mirror whenever I perform npm update.

@andycraig
Copy link
Collaborator

@renkun-ken I will wait until the npm package question is resolved before updating the license because I think some of the files being discussed are third party ones. (I think - I don’t know much about npm package management.)

@Ikuyadeu
Copy link
Member

Ikuyadeu commented Dec 17, 2019

Caused my environment, I could not check the View function.
My works are

  • Use npm package for jquery
  • Remove cross-references between extension and session
    It can be used by executing
npm install

@renkun-ken Could you check these changes will work or not on your environment?

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 17, 2019

@Ikuyadeu I test it and it does not work when package is built since the jquery, bootstrap, and jquery.json-viewer are not included in the built vsix package through vsce package.

When I install the vsix package on a remote machine, start R, and call View(mtcars), the WebView is blank and the debugger says

Error: Unable to read file (EntryNotFound (FileSystemError): Error: ENOENT: no such file or directory, open '/home/ken/.vscode-server/extensions/ikuyadeu.r-1.1.9/node_modules/jquery/dist/jquery.min.js')

I also go to the extension folder, and there's no node_module in the folder, which is supposed to only needed in development. And to deploy the JS/CSS packages to client-side, developer needs to customize the build command so that the scripts are copied to dist folder, something like what is mentioned in https://stackoverflow.com/questions/18245709/how-to-copy-from-node-modules-just-the-files-needed-for-distribution-using-grunt.

For DataTables package, we can use npm install datatables.net-bs4 (which is the version we are using) to get rid of resources folder entirely.

Update:

It looks like we should use a tool like https://github.com/webpack-contrib/copy-webpack-plugin to perform the task of copying JS/CSS from node_modules to dist.

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 17, 2019

@Ikuyadeu I use copy-webpack-plugin to manage the distribution of the JS/CSS resources used at client side and it works for me. The used resources will be copied to dist/resources at build time so that WebView can find those resources at runtime. Please take a look and feel free to modify. Now all external JS/CSS resources are manged under npm.

@Ikuyadeu
Copy link
Member

@renkun-ken Great! I will check again.

@renkun-ken
Copy link
Member Author

@andycraig Please take a look at the license issue. Do we have to do anything if we use npm to manage these external JS/CSS packages (we use many others too) and distribute them at build time?

@andycraig
Copy link
Collaborator

@renkun-ken I had a look through the PR files again and I can't see any copyright lines anymore. So, it's my understanding that there is now no code copied directly from other projects. If that's correct, then there's no need to update the LICENSE file. In which case, it's a LGTM. Thank you!

@renkun-ken
Copy link
Member Author

renkun-ken commented Dec 18, 2019

One thing I need to fix is that sorting in View(data) performs string sorting if a column contains NA. Seems like this can be fixed by https://datatables.net/reference/option/columns.type.

@renkun-ken
Copy link
Member Author

@Ikuyadeu The column type issue is fixed. Now this PR looks good to merge.

@renkun-ken renkun-ken mentioned this pull request Dec 18, 2019
@Ikuyadeu Ikuyadeu merged commit 0987432 into REditorSupport:master Dec 19, 2019
@Ikuyadeu
Copy link
Member

Ikuyadeu commented Dec 19, 2019

@renkun-ken Great! I merged now.
I will publish the new version this weekend, after refactoring the whole of the project.
(Of course, it also include update CHANGELOG.md about your contribution)

@renkun-ken
Copy link
Member Author

Thanks! @Ikuyadeu @andycraig

@andycraig
Copy link
Collaborator

Great piece of work @renkun-ken!

@toxintoxin
Copy link

I think I read somewhere that it seems to be possible to display each column of a tbl/df directly in the sidebar as a drop-down list, but I can't find that page anymore😭

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.

None yet

6 participants