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

Allow modules to return a value and access each other's value. #707

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

samussiah
Copy link
Contributor

No description provided.

@jwildfire jwildfire self-requested a review March 8, 2023 14:38
Copy link
Contributor

@jwildfire jwildfire left a 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(
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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) {
Copy link
Contributor

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 ...

Copy link
Contributor Author

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.


#Setting tab
callModule(settingsTab, "settings", domains = domainData, metadata=meta, mapping=current_mapping, charts = charts)

return(module_outputs1)
Copy link
Contributor

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

)

server <- function(input, output, session) {
module_outputs <- callModule(
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

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