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.initializedPromise #4063

Merged
merged 25 commits into from
Jul 24, 2024
Merged

Add Shiny.initializedPromise #4063

merged 25 commits into from
Jul 24, 2024

Conversation

wch
Copy link
Collaborator

@wch wch commented May 23, 2024

This PR adds Shiny.initializedPromise, which is a Promise-like object.

  • Shiny.initializedPromise 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 the event is triggered, then you need to create a jQuery event listener for the shiny:sessionintialized event. If your code executes after the event, 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.initializedPromise.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.initializedPromise.then(() => {
        log("static content: initializedPromise resolved!")
      });
      $(document).on("shiny:sessioninitialized", () => {
        log("static content: shiny:sessioninitialized event!")
      });      
    '))
  ),
  server = function(input, output) {
    output$dynui <- renderUI({
      tags$script(HTML('

        Shiny.initializedPromise.then(() => {
          log("dynamic content: initializedPromise resolved!")
        });
        $(document).on("shiny:sessioninitialized", () => {
          log("dynamic content: shiny:sessioninitialized 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
@jcheng5
Copy link
Member

jcheng5 commented Jul 23, 2024

I'm not a fan of the names isConnected and isInitialized, that would make me assume it's going to return a boolean. @gadenbuie and I had a similar discussion yesterday, he ended up with the equivalent of getConnectedPromise, I think my other suggestions were waitForConnect and joinConnected.

@jcheng5
Copy link
Member

jcheng5 commented Jul 23, 2024

What does isConnected do if you call it after the app has been disconnected? You just get the resolved promise, I assume... What about on reconnection?

I think I understand what the initialized one is for, I maybe am less sure what the connected one is for. The thing where Shiny.setInputValue and other Shiny methods aren't available, is that before initialization or before connected?

@wch
Copy link
Collaborator Author

wch commented Jul 23, 2024

  • I'm fine with dropping isConnected and just keeping isInitialized. This avoids the issue of what to do on a reconnection. And the shiny:connected event is not really useful for most cases anyway.

  • I don't have an attachment to isInitialized. The name getInitializedPromise implies that it's a method which returns a promise, but that's not what it is; it's the promise itself. I think the name initializedPromise would work, and is a fairly conventional name.

@wch wch changed the title Add Shiny.isConnected and Shiny.isInitialized Add Shiny.initializedPromise Jul 23, 2024
Comment on lines -8 to -11
// Tell Shiny variable globally exists
// eslint-disable-next-line @typescript-eslint/naming-convention
const 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.

Removing the global Shiny will change how other TypeScript code refers to the Shiny object. They can do use either of the following:

  • window.Shiny
  • import { Shiny } from "rstudio-shiny/srcts/types/src"

Note that this is a type change, so although it could make other TS code unhappy, it will not break any JS code.

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

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

// eslint-disable-next-line @typescript-eslint/naming-convention
let Shiny: ShinyClass;
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 Shiny object is exported; then the parent (at srcts/src/index.ts) re-exports it. Either of these references can be used to access the Shiny object, without needing to use window.Shiny or a global Shiny.

@wch wch merged commit bb89cf9 into main Jul 24, 2024
12 checks passed
@wch wch deleted the init-promise branch July 24, 2024 03:11
@github-actions github-actions bot restored the init-promise branch July 24, 2024 03:13
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.

2 participants