Align RCloud overrides with new shiny

att / rcloud.shiny

Shiny support for RCloud

Other

4 stars 12 forks source link

Align RCloud overrides with new shiny #32

Closed useless5771 closed 6 years ago

useless5771 commented 6 years ago

Re-applied RCloud-specific changes to Shiny 1.0.1 release sources. There were just a few changes, please refer to specific commits for details.

Also I performed some improvements to the way RCloud changes are applied to shiny so a pull request to shiny project can be raised. The main difference is that there are the following new 'shiny' options introduced:

shiny.uiHttpHandler
shiny.htmlDependencies
shiny.serviceAppRunner
shiny.showcase.codeTabs

The options hold function references and will allow (if shiny PR is merged) shiny package clients (RCloud) to customise its behaviour.

Additional change is that notebook's assets now are listed in the 'Assets' dropdown when shiny App is executed in 'showcase' mode within RCloud.

FIXES #29

gordonwoodhull commented 6 years ago

Thanks @useless5771. In your opinion, what else would we need to do in order to submit a respectable PR to Shiny?

useless5771 commented 6 years ago

I would suggest the following approach:

[x] merge this PR
[ ] update changes in https://github.com/att/rcloud.shiny/pull/20 and make sure that it works in R 3.5.x and then merge it,
[ ] fork shiny project repository
[ ] apply changes to our shiny fork that we want to share,
[ ] address any issues found (both in shiny fork and RCloud->rcloud.shiny->shiny integration)
[ ] making sure that rcloud.shiny can work with both, vanilla shiny and our fork (there could be a separate rcloud.shiny branch to verify that rcloud.shiny works with our shiny fork)
[ ] if our shiny fork is proven to satisfy our needs, we raise a PR to upstream repository.

gordonwoodhull commented 6 years ago

I diffed the overrides against Shiny 1.1.0 (which is what I think you meant above). This approach is really nice and clean!

I'm merging this as rcloud.shiny 0.6.

Your plan above makes a lot of sense. I'm not sure if I agree with the penultimate step, however - it seems to me we can make a clean break and not worry about vanilla shiny anymore. If they accept our PR, then that's great. Otherwise we back out to what we have here, which is still a better design. I do agree we would want to keep any work that relies on a modified Shiny in a branch.

gordonwoodhull commented 6 years ago

Thanks @useless5771!