-
Notifications
You must be signed in to change notification settings - Fork 24
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
Allow modules to return a value and access each other's value. #707
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few questions for you @samussiah. In short, I think I'd scale this back a little bit for now. We can think about a more complex implementation later.
Also need to update the @value in the roxygen header to describe what is being returned.
Happy to chat if needed.
module_outputs | ||
) | ||
} else { | ||
module_output <- output[["chart-wrap"]] <- chart$functions$server( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is anything actually being returned here for non-shiny charts? I guess in theory the main
function could return something, but is this just NULL for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, nothing now... I can imagine returning a filtered/transformed dataset or a population subset.
R/mod_chartsTab.R
Outdated
@@ -27,7 +27,7 @@ chartsTabUI <- function(id, chart){ | |||
#' | |||
#' @export | |||
|
|||
chartsTab <- function(input, output, session, chart, data, mapping){ | |||
chartsTab <- function(input, output, session, chart, data, mapping, module_outputs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that this is all of outputs from the other charts? Is this reactive? Are you using this for anything right now? Wondering if we could trigger an infinite loop with this somehow ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm imagining a module that aggregates output from other modules, each with its own set of inputs.
mapping=current_mapping | ||
) | ||
) | ||
module_outputs <- reactiveValues() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reactiveValues
is actually what's working. It allows passing around a single, updateable object of reactive modules.
chart=chart, | ||
data=filtered_data, | ||
mapping=current_mapping, | ||
module_outputs=module_outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be module_outputs1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the module_outputs
of returned map
output.
R/mod_safetyGraphicsServer.R
Outdated
|
||
#Setting tab | ||
callModule(settingsTab, "settings", domains = domainData, metadata=meta, mapping=current_mapping, charts = charts) | ||
|
||
return(module_outputs1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to return this from the overall server function ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
R/safetyGraphicsApp.R
Outdated
) | ||
|
||
server <- function(input, output, session) { | ||
module_outputs <- callModule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we want to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have the server function return something, I'd think it should be a larger config including meta
, mapping
etc. For now, I don't think the overall server function is well designed to be used as part of a larger app, so I'd lean towards not implementing this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
No description provided.