654 pass reactive data@main

[gogonzo]

closes #654

1. Passing list of reactive data to the teal_module if has data argument. I'm passing the list with attributes as I think it's easier than list(data = list(ADSL = reactive(), ...), attributes = list(expr = reactive(), ...)).
2. changes the validation of the teal_module which was quite strict by requiring ordered arguments, always need datasets or .... We can ease this requirement because some modules might not need datasets at all. in ui_nested_tabs and srv_nested_tabs we use do.call which pass args by their name (doesn't care about order)
3. Extracted some code from get_rcode which combines DDL with filter expr

---

I found some inconsistency. For example:

• In the filter-panel we turn ADSL -> ADSL_FILTERED but everywhere we still use unchanged datanames. In data_merge we use hardcoded _FILTERED which means that "independent" data_merge still depends on the environment where ADSL_FILTERED exist (but we use dataname = "ADSL")

---

Sample app

mod_data <- teal::module(
  ui = function(id) {
    ns <- NS(id)
    verbatimTextOutput(ns("test"))
  },
  server = function(id, data) {
    shiny::moduleServer(id, function(input, output, session) {
      output$test <- renderText({
        paste(attr(data, "code")(), collapse = "\n")
      })
    })
  },
  label = "mod_data",
  filters = "all",
  server_args = NULL,
  ui_args = NULL
)


app <- init(
  data = teal_data(
    dataset("IRIS", iris, code = "IRIS <- iris"),
    dataset("MTCARS", mtcars, code = "MTCARS <- mtcars"),
    check = TRUE
  ),
  modules = modules(example_module(), mod_data),
  header = "My first teal application"
)

runApp(app)

Sample app with lightweight reproducible object

devtools::load_all("teal")

eval_code <- function(data, code) {
  # some assertions for data and code
  env <- list2env(
    sapply(data, simplify = TRUE, function(x) if (is.reactive(x)) x() else x)
  )
  eval(expr = parse(text = code),  envir = env)
  for (i in ls(env)) {
    data[[i]] <- env[[i]]
  }
  attr(data, "code") <- c(attr(data, "code"), code)
  data
}

mod_data <- teal::module(
  ui = function(id) {
    ns <- NS(id)
    div(
      verbatimTextOutput(ns("test")),
      tableOutput(ns("filtered"))
    )
  },
  server = function(id, data) {
    shiny::moduleServer(id, function(input, output, session) {
      react1 <- reactive(eval_code(data, "new_data <- IRIS"))
      react2 <- reactive(eval_code(react1(), "new_data <- head(new_data)"))
      output$test <- renderText(attr(react2(), "code") |> paste(collapse = "\n"))
      output$filtered <- renderTable(react2()[["new_data"]])
    })
  },
  label = "mod_data",
  filters = "all",
  server_args = NULL,
  ui_args = NULL
)


app <- init(
  data = teal_data(
    dataset("IRIS", iris, code = "IRIS <- iris"),
    dataset("MTCARS", mtcars, code = "MTCARS <- mtcars"),
    check = TRUE
  ),
  modules = modules(example_module(), mod_data),
  header = "My first teal application"
)

runApp(app)

[nikolas-burkoff]

A couple of comments from me

[nikolas-burkoff]

is_reporter_used works on module and modules objects (it's an S3 method) we should probably rename is_reporter_used to is_module_arg_used and pass in "reporter", "dataset", "id" etc. so we get consistent behaviour here?

[gogonzo]

Good point

[Polkas]

we will need is_module_ui_arg_used and is_module_srv_arg_used to cover all scenarios. Or add an argument is_module_arg_used(..., type = "srv" or "ui").

[gogonzo]

@nikolas-burkoff @Polkas @mhallal1 @kpagacz
I just want to highlight one thing. There were multiple options of how we should call modules to maximize simplicity.

# 1. use expr, join_keys argument
srv_custom <- function(data = list(...), expr = <expr>, join_keys = <join_keys>, ....)

# 2. use attributes
srv_custom <- function(data = structure(list(...), expr = <expr>, join_keys = <join_keys>, ...)

# 3. use nested list 
srv_custom <- function(data = list(data = list(....), expr = <expr>, join_keys = <join keys>, ...)

Together with @pawelru we think that nested list (3) is to "complicated" to put reactive data into data element of the list. We debated about (1) and (2) and we concluded that having expr and join keys as attribute is optimal solution. here is why:

• expr is a code to reproduce data and it's stricltly connected to data.
• join_keys is entity of the data which helps to define relationships between datasets. Also including dm would make this argument irrelevant as dm already have a join_keys
• In the future our "chunks" will be a simple object which will be a env/list + expr which should be accessed by [[, $, [, getElement and evaluated by simply eval_code(<chunks>, <expr>). Having list + attr everywhere will be easier for users to understand how to construct NEST objects.
• having expr and join_keys as arguments of the module will require us to conditionally pass them in (same as reporter, data etc.).

Please let me know what is your opinion about this.

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -----------------------------------------------
R/default_filter.R                   7       7  0.00%    17-27
R/dummy_functions.R                 74      61  17.57%   12-95
R/example_module.R                  17      17  0.00%    19-35
R/get_rcode_utils.R                 52       2  96.15%   94, 99
R/get_rcode.R                      131      40  69.47%   66, 73, 78, 212-261
R/include_css_js.R                  20       0  100.00%
R/init.R                            39      21  46.15%   171, 182-183, 236-257
R/log_app_usage.R                   38      38  0.00%    34-119
R/logging.R                         13      13  0.00%    11-28
R/module_nested_tabs.R             110      15  86.36%   57, 96, 115-127, 163, 212, 252
R/module_tabs_with_filters.R        52      21  59.62%   111-137
R/module_teal_with_splash.R         33       2  93.94%   62, 74
R/module_teal.R                    119      28  76.47%   49, 52, 142-143, 156-162, 168-174, 197, 227-239
R/modules_debugging.R               19      19  0.00%    41-60
R/modules.R                        109      10  90.83%   218, 287, 415-440
R/reporter_previewer_module.R       12       2  83.33%   18, 22
R/show_rcode_modal.R                20      20  0.00%    17-38
R/utils.R                            6       0  100.00%
R/validations.R                     62      39  37.10%   103-355
R/zzz.R                             11       7  36.36%   3-14
TOTAL                              944     362  61.65%

Results for commit: 54545cf

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

    1 files    10 suites   12s ⏱️
104 tests 104 ✔️ 0 💤 0 ❌
205 runs  205 ✔️ 0 💤 0 ❌

Results for commit 7d1c209.

♻️ This comment has been updated with latest results.

[nikolas-burkoff]

A few comments from me

[mhallal1]

can we move this check into the if-statement in line 95 to avoid checking for it when not needed.
is checkmate::assert_class needed for a more strict check?

[gogonzo]

ui_nested_tabs needs FilteredData and FilteredData will still be delivered here for a long time in the same shape. nested_tabs is the place where convertion from FIlteredData -> data happens.

Also, nested_tabs is not exported also which means that we don't have to simplify api for this one.

[gogonzo]

ui_nested_tabs needs FilteredData and FilteredData will still be delivered here for a long time in the same shape. nested_tabs is the place where convertion from FIlteredData -> data happens.

Also, nested_tabs is not exported also which means that we don't have to simplify api for this one.

[mhallal1]

passing data and datasets to the same module seems to be possible also, is this by design?

[nikolas-burkoff]

@gogonzo @mhallal1 - this will be right, but I'm super confused about how this interacts with the merge expression which also has filtering in? No need to explain I guess as this all is being changed

[nikolas-burkoff]

This looks good to me, a few minor comments. I'll let @mhallal1 approve as he's been much closer to this than me

[mhallal1]

LGTM