shinyscreenshot icon indicating copy to clipboard operation
shinyscreenshot copied to clipboard

Can you add the check to suppress getDependencies() in screenshotButton()?

Open acahill opened this issue 3 years ago • 5 comments

Hi

Thanks for writing this fantastic library. I use it to generate screenshots of charts with all of the extra bits that were left out by Plotly's screenshot feature.

I'd like to add shinyscreenshot's client dependencies to my application's Webpack bundle, but I couldn't figure out how to stop getDependencies() being called at runtime. I tried setting the flag in my app.R:

server <- function(input, output, session) {
  session$userData$.shinyscreenshot_added <- TRUE
  ...
}

But the three js resources are still being loaded. While writing this I had a closer read of screenshot.R and I can see now the flag being checked and set in screenshot() is not checked in screenshotButton(). Any chance you can add it?

Alternatively, it might make sense to add an option like load_dependencies = TRUE to both functions. In the meantime I can work around the issue by making my own version of screenshotButton()

Thanks again!

acahill avatar May 31 '21 00:05 acahill

I don't fully understand the problem/when an issue is happening.

But your solution of session$userData$.shinyscreenshot_added <- TRUE sounds like it should work when using the screenshot() function, doesn't it? screenshotButton() is just some syntactic sugar but has no functional difference from creating a button and calling screenshot(), so can you use screenshot() instead of the button version?

Alternatively, I can consider changing the current dependency system to htmltools::htmlDependency() - would that solve your issue?

Regarding your two suggestions, unfortunately I don't think I would be ok with them. Adding a check to screenshotButton() when there is no way for the code to get to that situation (other than from a user monkey patching) doesn't seem like a correct thing to do. And I'm not too keen on adding a load_dependencies parameter because it seems extremely esoteric and I'm very conservative about functions becoming laundry lists of parameters to support single users (as you might imagine, I get a lot of private messages asking me to add parameters to different functions, so I need to be very defensive of it)

daattali avatar May 31 '21 06:05 daattali

Hi. That sounds reasonable, thanks for looking into it for me. I had assumed it was a bug because the two very similar functions screenshot and screenshot button have different behaviour; screenshot may call getDependencies, but screenshotButton always calls it.

Setting that flag works with screenshot, but doesn't help with screenshotButton. Since I wanted a screenshot button, for now I have just copied and pasted the definition from screenshot.R into my application, deleted the call to getDependencies on line 138, and that worked as expected.

I see what you mean about the extra parameter. Would using htmlDependency mean it could be suppressed with suppressDependencies? If so then that sounds perfect to me. Cheers!

acahill avatar May 31 '21 08:05 acahill

I forgot another very important point regarding checking session$userData$.shinyscreenshot_added inside the button function: that function is a UI function. It gets called from a UI, not from a server. There is no session object to even check (unless it's being called from a renerUI/insertUI).

The screenshot() function makes that check not because it has to, but for performance reasons: if it's the first time it's being called, I need to make an additional call to insertUI() so I explicitly want to limit that to only once rather than every time the function is called. But screenshotButton() doesn't need to do that check because it's already adding UI anyway.

Using htmlDependency may or may not solve your issue (yes, with suppressDependencies). If you think that approach works for you, as long as it doesn't break anything, I'd be ok with a PR if you want to try it out and see if it works and submit one

daattali avatar May 31 '21 16:05 daattali

Here's some documentation for how to use htmlDependency if you want to try to tackle that https://shiny.rstudio.com/articles/packaging-javascript.html#htmldependency-object

daattali avatar May 31 '21 16:05 daattali

Thanks! That's interesting about the performance aspect of the server function, makes sense. Anyway, I'll see what I can do with htmlDependency

acahill avatar May 31 '21 22:05 acahill

@acahill I did end up switching to using html dependencies in #20 and now you can use suppressDependencies. If you install the latest github version, this should work for you:

fluidPage(
  screenshotButton(...),
  htmltools::suppressDependencies("html2canvas-js", "filesaver-js", "shinyscreenshot-binding"),
)

It would be nicer if you could do htmltools::removeDependencies(screenshotButton(...)) but that sort of functionality doesn't exist - https://github.com/rstudio/htmltools/issues/352

daattali avatar Nov 09 '22 19:11 daattali

Hey thanks, that looks great!

acahill avatar Nov 09 '22 21:11 acahill

I wasn't sure if you're still on github, glad you're around. If you can, please do report back letting me know that it indeed works (or not)

daattali avatar Nov 09 '22 22:11 daattali