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

Revert Shiny.renderContent() back to sync instead of async #3929

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Oct 27, 2023

Motivated by rstudio/bslib#872

This PR essentially reverts the changes made to srcts/src/shiny/render.ts in #3904, then does:

  1. Keeps the shinyBindAll(el) -> await shinyBindAll(el) change made to renderContentAsync()
  2. Adds a shinyBindAll(el) -> return shinyBindAll(el) change in renderContent() (so you can await the bind if need be)

@cpsievert cpsievert marked this pull request as ready for review October 27, 2023 22:40
@cpsievert cpsievert requested review from wch and gadenbuie October 27, 2023 22:40
@wch
Copy link
Collaborator

wch commented Oct 28, 2023

If we were to use the synchronous code path for loading these resources, then it won't work in shinylive on Chrome (because Chrome's service worker implementation doesn't intercept synchronous XHR requests, which is what jQuery users for synchronous loading of JS resources).

I'll comment on the bslib issue about possible ways forward there.

@@ -79,22 +79,48 @@ async function renderContent(
}
}

// This function was introduced in v1.7.5, then deprecated in v1.8.0 once we
Copy link
Collaborator

@wch wch Oct 30, 2023

Choose a reason for hiding this comment

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

For renderContent, please add some information about why this won't work in shinylive (and about the synchronous XHR warnings even outside of shinylive, #789). Also explain what parts of this function happen synchronously, and what part (the bindAll is async, and how that could be an issue in some cases.

And for renderContentAsync, tell people that this is what they should use going forward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a pretty good (and lengthy) comment just above renderContentAsync() on this topic

@wch wch merged commit 56878eb into main Oct 30, 2023
11 checks passed
@wch wch deleted the renderContentSync branch October 30, 2023 17:59
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