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

Add Shiny.isConnected and Shiny.isInitialized #4063

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add Shiny.isConnected and Shiny.isInitialized #4063

wants to merge 18 commits into from

Conversation

wch
Copy link
Collaborator

@wch wch commented May 23, 2024

This PR adds Shiny.isConnected and Shiny.isInitialized, which are Promise-like objects.

  • Shiny.isConnected is resolved when the shiny:connected event is triggered.
  • Shiny.isInitialized is resolved when the shiny:sessionintialized event is triggered.

The problem with the previous way of doing things, is that there needs to be two paths in the code: If your code executes before those events are triggered, then you need to create a jQuery event listener for the shiny:connected or shiny:sessionintialized events. If your code executes after those events, then you need to just run your function. And detecting whether the event has happened is not straightforward.

With the new method, you can simply write this code, and it will work in both situations:

Shiny.isInitialized.then(() => {
  // Do something here
});

See below for an example.

This pull request also:

  • Converts window.Shiny to a normal JS class object.
  • Adds a prettier plugin which sorts imports
  • Simplifies some types in the Shiny class.

The following example app demonstrates how the promises work and compares them to the event listeners. The promises chains and event listeners are set up in both the static UI and in dynamic UI.

For the static UI, all of the callbacks will execute. But for the dynamic UI, only the promise callbacks will execute; the event listeners will not. This is because the dynamic UI code runs after those events have already been triggered.

shinyApp(
  ui = fluidPage(
    "Initialization test app",
    uiOutput('dynui'),
    pre(id="logs"),
    tags$script(HTML('
      function log(s) {
        document.getElementById("logs").innerText += s + "\\n";
      };
    
      Shiny.isConnected.then(() => {
        log("static content: isConnected resolved!");
      });
      Shiny.isInitialized.then(() => {
        log("static content: isInitialized resolved!")
      });
      $(document).on("shiny:sessioninitialized", () => {
        log("static content: shiny:sessioninitialized event!")
      });      
      $(document).on("shiny:connected", () => {
        log("static content: shiny:connected event!")
      });      
    '))
  ),
  server = function(input, output) {
    output$dynui <- renderUI({
      tags$script(HTML('

        Shiny.isConnected.then(() => {
          window.log("dynamic content: isConnected resolved!")
        });
        Shiny.isInitialized.then(() => {
          log("dynamic content: isInitialized resolved!")
        });
        $(document).on("shiny:sessioninitialized", () => {
          log("dynamic content: shiny:sessioninitialized event!")
        });      
        $(document).on("shiny:connected", () => {
          log("dynamic content: shiny:connected event!")
        });      
      '))
    })
  }
)

If you open this app in a browser, this is what you will see:
image

@wch wch requested a review from schloerke May 23, 2024 18:05
.eslintrc.yml Show resolved Hide resolved
Comment on lines -18 to -19
// Make `Shiny` a globally available type definition. (No need to import the type)
type Shiny = RStudioShiny;
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 type didn't seem to be used anywhere, and it was confusing that the type and the variable had the same name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should check if bslib complains about this change.

import { setUserAgent } from "../utils/userAgent";
import { windowUserAgent } from "../window/userAgent";

import { initReactlog } from "../shiny/reactlog";

function init(): void {
setShiny(windowShiny());
window.Shiny = window.Shiny || new Shiny();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this captures what the previous setShiny() function was doing, but I am not 100% sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One reason that we previously didn't want to simply overwrite the window.Shiny object was because some other library might create a window.Shiny = {} object and then populate some of the properties.

This is the old code in this repo that created the window.Shiny object this way. https://github.com/rstudio/shiny/blob/v1.4.0/srcjs/_start.js

  var exports = window.Shiny = window.Shiny || {};

However, in practice it looks like, at least for shiny-server-client, its JS must get loaded after shiny.js, so the change in the PR shouldn't cause any behavioral changes:
https://github.com/rstudio/shiny-server-client/blob/92cbc96583f958f4e379120323fb3a48c2e61fd5/lib/main.js#L167-L187

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From a conversation with @schloerke: It would be better to just print a console error message if window.Shiny already exists at this point.

srcts/src/shiny/index.ts Show resolved Hide resolved
Comment on lines 520 to 528
$(document).one("shiny:connected", (event) => {
initDeferredIframes();
// @ts-expect-error; .socket property isn't a known property of
// JQuery.TriggeredEvent, but it was added on there anyway.
windowShiny._resolveConnectedPromise(event.socket);
});

$(document).one("shiny:sessioninitialized", () => {
windowShiny._resolveSessionInitPromise();
Copy link
Collaborator Author

@wch wch May 23, 2024

Choose a reason for hiding this comment

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

The promises are resolved via event listeners, but one could argue that they should be resolved directly from the initialization code. This would be more direct, and let us avoid going through the jQuery event system.

One small drawback of going that route is that the promise resolvers would need to be passed down to the shinyapp object.

@wch wch requested a review from jcheng5 May 23, 2024 18:13
srcts/src/shiny/index.ts Outdated Show resolved Hide resolved
@wch wch mentioned this pull request May 24, 2024
1 task
}
type Shiny = RStudioShiny;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The removal of this type leads to some required changes in bslib.
Note that the change is only with types; the JS code does not need to change.
rstudio/bslib#1054

@wch wch force-pushed the init-promise branch 2 times, most recently from d4aa481 to 561a9a1 Compare May 24, 2024 16:49
@wch wch force-pushed the init-promise branch 2 times, most recently from 5d1e2eb to f76c5b1 Compare May 30, 2024 16:55
@wch wch changed the title Add Shiny.connectedPromise and Shiny.sessionInitPromise Add Shiny.isConnected and Shiny.isInitialized May 30, 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.

None yet

2 participants