shiny Avoid jQuery using eval() for dependency JS

This allows to use debuggers for dependency code.

Oct 17 '17 10:10 flying-sheep

Can you provide some information about the problem, and an example of this in action?

Oct 17 '17 15:10 wch

jQuery evals <script/> tags on first insertion.

but if you insert them normally instead, they’re automatically executed as well. the difference (and the reason why jQuery does this) is that after calling $something.append(script), you could theoretically directly use the variables defined in the script.

a demonstration of why there is a problem is here. screenshot:

grafik

note how the native variant has the script URL instead of “anonymous” and lacks the rat tail of jQuery machinery? that’s because the browser downloaded and executed the script, and therefore knows about its URL. now if i want to debug code, the latter is vastly preferable, as i can actually jump to the source, step in and out, …

Oct 18 '17 11:10 flying-sheep

It looks like the jQuery .append() loads and evals code synchronously, while .appendChild() will do it asynchronously. This change would affect not just debugging, but possibly the behavior of applications.

This may be an improvement in some cases, but it also may cause changed behavior in others, so we'll have to look carefully at the consequences before we take it.

Oct 19 '17 15:10 wch

fixed in 7532ca53d951cfab7054409ef81d6557e26b2e69. quoting MDN:

Dynamically inserted scripts execute asynchronously by default, so to turn on synchronous execution (i.e. scripts execute in the order they were loaded) set async=false.

if all inserted script tags are inserted with async=false, they’ll be executed in order as before, making the variables from earlier scripts available in later ones.

the only difference is that the variables aren’t available

in the script that called renderDependency. but i assume that’s called automatically by shiny, and never in user code.
in <script>code here</script> tags that are inserted. but that’s no problem: bigger things should be done by inserting scripts, the rest with messages.

Oct 20 '17 06:10 flying-sheep

Some issues that came up in a discussion of this PR:

Many widgets require the JS dependencies to be loaded synchronously, because the initialization for the widget happens right after the HTML is written to the DOM, and it expects the JS library to be loaded. Here's the slider, for example.
If the widgets are written in the app's starting UI (as opposed to content added by uiOutput() or htmlOutput()), then the <script> tags are already part of the document's DOM. In these cases, the debugging already works as you want.
the .async property is supported by all modern browsers, but notably, not IE 9: http://caniuse.com/#search=async. I believe this means that this PR would break applications with dynamic UI on IE 9. We are no longer actively supporting IE 9, but neither do we want to introduce a change that breaks it, unless there's a really good reason. Even setting that aside, it would be important to test many other browsers to make sure this doesn't break Shiny on them.

One trick I use when I want to be able to jump into JS code that's loaded this way is to put a console.log() in it somewhere. This prints out to the console and most browsers provide a link to the line of code where the console.log() was called; clicking on it will take you to the code.

It might make sense in the future to provide an option for an htmlDependency to have its scripts loaded asynchronously.

Oct 25 '17 00:10 wch

hi, i think those issues can be solved:

Many widgets require the JS dependencies to be loaded synchronously, because the initialization for the widget happens right after the HTML is written to the DOM, and it expects the JS library to be loaded.

i don’t understand: loading multiple scripts by attaching script nodes to the <head> and setting their .async = false means they’ll load in sequence (i.e. $el.ionRangeSlider(opts) will work if $.fn.ionRangeSlider is defined in a script attached earlier)

so if there’s a problem, it’s probably just that some script isn’t inserted with .async = false, right?

I believe this means that this PR would break applications with dynamic UI on IE 9

then let’s add a helper that uses jquery’s .append on IE9 and this method on other browsers, OK?

One trick I use when I want to be able to jump into JS code that's loaded this way […]

all links and debugging are broken in firefox.

Oct 25 '17 07:10 flying-sheep

i don’t understand: loading multiple scripts by attaching script nodes to the <head> and setting their .async = false means they’ll load in sequence (i.e. $el.ionRangeSlider(opts) will work if $.fn.ionRangeSlider is defined in a script attached earlier)

Yes, I was just describing why synchronous loading is necessary. The .async=false should work for most browsers.

then let’s add a helper that uses jquery’s .append on IE9 and this method on other browsers, OK?

That sounds reasonable. It'll still need to be tested on many browsers though, just to make sure that everything works.

One trick I use when I want to be able to jump into JS code that's loaded this way […]

all links and debugging are broken in firefox.

Ah, apparently that's a Chrome feature. For Firefox and Safari, you have to add a special comment that gives the code a pseudo-filename:

eval(`var x = 1;
    console.log('x is ' + x);
    //# sourceURL=myscript.js`);

Firefox doesn't support clicking on the console.log() output to go to the source, but Safari does. (I'm using the latest Developer edition of Firefox.)

Oct 25 '17 15:10 wch

For Firefox and Safari, you have to add a special comment that gives the code a pseudo-filename

that would be an alternative route. we could just add the path to the script this way!

Oct 25 '17 20:10 flying-sheep

Here's a self-contained test app we can use to see if dependencies are indeed loading synchronously. It just sets window._dependency_loaded_ in an htmlDependency script, immediately followed by a <script> that looks for that field. This change looks good in the latest Chrome, Safari, and Firefox on Mac (note that you need to run grunt first, so you can't just install from this branch).

library(shiny)

ui <- fluidPage(
  uiOutput("ui")
)

server <- function(input, output, session) {
  output$ui <- renderUI({
    dir <- tempfile()
    dir.create(dir)
    writeLines("window._dependency_loaded_ = true;", file.path(dir, "dep.js"))
    
    tagList(
      htmltools::htmlDependency(paste0("dep", sample.int(900000, 1)),
        version = "1.0",
        src = dir,
        script = "dep.js"),
      tags$script("alert(window._dependency_loaded_ ? 'success' : 'failure');")
    )
  })
}

shinyApp(ui, server)

Oct 25 '17 22:10 jcheng5

Huh. Now it's failing for me every time.

I pushed a branch with this change + rebuilt JS files: devtools::install_github("rstudio/shiny@joe/bugfix/flying-sheep-patch")

Oct 25 '17 23:10 jcheng5

@jcheng5 it's failing for me in RStudio and in Chrome (Mac), when I use your branch.

According to MDN, .async=false only ensures that the scripts will be executed in the order that they're loaded -- not that it will execute at the moment that it's inserted in the page.

Dynamically inserted scripts execute asynchronously by default, so to turn on synchronous execution (i.e. scripts execute in the order they were loaded) set async=false

I think the problem is that the inline javascript (the alert()) does NOT have async=false, and so it's not subject to the ordering guarantee. And since it loads inline instead of via src, setting the value of async has no effect on it.

Oct 26 '17 16:10 wch

not that it will execute at the moment that it's inserted in the page

when else?

Oct 26 '17 21:10 flying-sheep

The next tick? On page load complete? Whenever it is, it's not at the moment it's inserted into the page, at least not on any browser I've tried.

Oct 26 '17 22:10 jcheng5

Here's an even more boiled down example that doesn't use Shiny:

Demo (Source)

It dynamically loads a dependency that alerts "Dependency loaded", followed by an inline script that alerts "Inline executed" and on the next tick alerts "Next tick". The order we need is:

Dependency loaded
Inline executed
Next tick

But instead it's always

Inline executed
Next tick
Dependency loaded

Oct 26 '17 22:10 jcheng5

shiny apps don’t need inline scripts. for static scripts, there’s <script src="..."/>, and for dynamic code, there’s session$sendCustomMessage(...).

Oct 27 '17 07:10 flying-sheep

The inline script example is just to show that it's not actually synchronous, despite async = false. I think any Shiny app that introduces, through renderUI/insertUI/insertTab, any inputs/outputs with previously-unseen HTML dependencies will have a problem. Here's an example:

library(shiny)

ui <- fluidPage(
  checkboxInput("showTable", "Show table"),
  uiOutput("out")
)

server <- function(input, output, session) {
  output$out <- renderUI({
    req(input$showTable)
    
    DT::dataTableOutput("table")
  })
  
  output$table <- DT::renderDataTable({
    cars
  })
}

shinyApp(ui, server)

This will also affect any Shiny Rmd that uses an htmlwidget.

Here's the code inside Shiny that assumes the scripts will load synchronously: https://github.com/rstudio/shiny/blob/ba8d79f2020adb7082b77a28c45877b433ea34e5/srcjs/output_binding_html.js#L43-L60 Line 43 is what loads the dependencies, and the calls to exports.initializeInputs and exports.bindAll below need them to have been successfully loaded.

Oct 27 '17 08:10 jcheng5

that it's not actually synchronous

of course, i was careful to point this out from the beginning.

Here's the code inside Shiny that assumes the scripts will load synchronously:

sorry, i’m not familiar enough to know what this does. what’s renderContent? is it for html dependencies or internal use? why do exports.initializeInputs and exports.bindAll need the scripts in the rendered html to have executed (i.e. what do they access)?

Oct 27 '17 09:10 flying-sheep

Sorry, I didn't understand your earlier message pointing out this behavior, but now I do. renderContent is called for all HTML assets that arrive dynamically in any fashion (renderUI, insertUI, insertTab). Those HTML assets are, conceptually, a fragment of HTML that may or may not have dependencies attached. After the content is inserted into the page (and dependencies fully loaded), initializeInputs and bindAll look at the newly inserted content and look for HTML elements that are Shiny inputs or outputs. The way this works is that Shiny custom binding classes (basically plugins, see [1, 2]) are interrogated one by one to see if there are any elements that are relevant to them. Since the HTML dependencies may contain the custom binding classes that are needed for the HTML that was just inserted, it's essential that the dependencies be loaded then.

The obvious next question is, why not defer initializeInputs and bindAll until the dependencies have asynchronously (but in-order) loaded? It would not be impossible but would certainly be a big refactor--in order to preserve the ordering of side effects of operations coming from the server, basically all of the operations that can be initiated from the server would have to be refactored to use a work queue, or promises, or something.

We'll look into the //# sourceURL= pragma approach though. Maybe we only insert it if there aren't existing sourcemap pragmas in the JS?

Oct 27 '17 18:10 jcheng5

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Oct 02 '19 15:10 CLAassistant