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

Adds search as a future for server-side rendering #1011

Closed
wants to merge 2 commits into from

Conversation

andyquinterom
Copy link
Contributor

Resolves #1010

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Please add a bullet to NEWS.md to mention this new feature (also feel free to add your name to DESCRIPTION as a ctb contributor). Thanks!

NAMESPACE Show resolved Hide resolved
R/shiny.R Outdated

toJSON = shinyFun('toJSON')
httpResponse = shinyFun('httpResponse')

filterFun = function(data, req) {
filterExpr = expression({
Copy link
Member

Choose a reason for hiding this comment

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

Usually I don't prefer using eval(expression()) because we need to be careful about the evaluating environment. Is it okay to still make a function here, and provide a function call in promises::future_promise()?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filterExpr = expression({
filterFun2 = function(data, req) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me

R/shiny.R Outdated
Comment on lines 584 to 586
filterFun = function(data, req) {
if (identical(future, FALSE)) return(eval(filterExpr))
else return(future_promise(seed = TRUE, { eval(filterExpr) }))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filterFun = function(data, req) {
if (identical(future, FALSE)) return(eval(filterExpr))
else return(future_promise(seed = TRUE, { eval(filterExpr) }))
filterFun = function(data, req) {
if (future) promises::future_promise(filterFun2(), seed = TRUE) else filterFun2()

@@ -56,7 +56,7 @@ DTOutput = dataTableOutput
#' a data object
renderDataTable = function(
expr, server = TRUE, env = parent.frame(), quoted = FALSE,
funcFilter = dataTablesFilter, ...
funcFilter = dataTablesFilter, future = FALSE, ...
Copy link
Member

Choose a reason for hiding this comment

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

The new argument needs to be documented.

@andyquinterom
Copy link
Contributor Author

I'll make the changes and document it asap

@andyquinterom
Copy link
Contributor Author

Changes made! @yihui

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

There are a few minor changes that I'd like to make before merging. Could you allow me to edit the PR? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork Thanks!

@andyquinterom
Copy link
Contributor Author

I'll have to create the Pull request again.

@yihui
Copy link
Member

yihui commented Aug 18, 2022

You don't really need to create a new PR. All you need to do is check the box "Allow edits by maintainers" at the bottom of the right sidebar of this page.

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.

Allow search to be done in a future
3 participants