Introduce the new DDL (module)

[gogonzo]

Part of #937
Closes #895

I introduce new type of module tm_teal_data (class teal_module_data), which allows to make any modification on existing teal_data object. Constructor of tm_teal_data is very similar to ddl

I included tm_teal_data in teal::init(modules) for one reason. Users might have some data created in pre-processing (before app starts) and some might be added in the interactive session. This solution prevents to load big objects in ddl which could be loaded before shiny starts - and thus not to influence app loading time.

Some key notes:

1. Module can contain any ui and server and must return modified (or not modified) teal_data object.
2. Module can be used as additional interactive step to load data (for example fixed global filter)
3. Since module doesn't have datanames and join_keys (returned reactive contains datanames and join_keys) some asserts needed to be moved further.

Example app code

library(scda)
pkgload::load_all("teal")

data_module <- tm_teal_data(
  label = "super data",
  code = '
    open_conn(username = input$username, password = input$password)
    ADSL <- scda::synthetic_cdisc_data("latest")$adsl
    ADTTE <- scda::synthetic_cdisc_data("latest")$adtte
  ',
  input_mask = list(
    username = quote(askpass::askpass()),
    password = quote(askpass::askpass())
  ),
  ui = function(id) {
    ns <- NS(id)
    tagList(
      textInput(ns("username"), label = "Username"),
      passwordInput(ns("password"), label = "Password"),
      actionButton(ns("submit"), label = "Submit")
    )
  },
  server = function(id, code, input_mask) {
    moduleServer(id, function(input, output, session) {
      eventReactive(input$submit, {
        masked_data <- teal_data() |> eval_and_mask(code = code, input = input, input_mask = input_mask)
      })
    })
  }
)

app <- init(
  data = data_module,
  modules = modules(
    teal.modules.general::tm_data_table(label = "yolo")
  ),
  filter = teal_slices(
    teal_slice(dataname = "ADSL", varname = "AGE", selected = c(18, 60)),
    teal_slice(dataname = "inexisting", varname = "AGE")
  )
)

runApp(app)

[kartikeyakirar]

we don't have ddl class anymore but code is still 'teal_module_data' . Could we check the presence of 'teal_module_data' in the 'modules' and retrieve the code from 'server_arg' ?

e.g

module_data <- extract_module(modules, "teal_module_data")
if (length(module_data) > 1L)  {
module_data$super_data$server_args$code  # seperate logic to extract code
}

[gogonzo]

I've made a push a code where data module is accepted in data . There is still a chance to include this to the modules and I will fix this in the evening. I will also try to split ddl and server. Thanks!

[gogonzo]

Another PR #957 with the @vedhav suggestion. WDYT?

[pawelru]

Out of all the available options we have I like this one the most. I think that the PR #957 (transform module) should be an extension of this (i.e. new constructor of teal_module_data based of teal_data alongside ui and srv) and not an alternative.
This is actually a big advantage of this approach - as long as we stick to the basic simple data structures (teal_module_data class) we can crate as many constructors as we like.
I would even suggest to go step further with simplifying inputs and allow a module (i.e. list with ui and srv elements). This will essentially allow users to create their own input objects. We just need to make assertions on the returned value out of this module - i.e. assert that returned object is of teal_data class. I really don't see the point of a separate teal_module_data (or ddl in separate PR) class. It unnecessarily complicates the life of app developers in case any customisation is needed. Note that this does not indicate that I don't see point of helper functions to create proper input objects. I just want it to be more open. Data input is a place where I do expect a lot of experiments and customisations.

(I do have a few implementation comments but I will hold off for now to first agree on the approach)

[m7pr]

Does it need any extra work on a removal of user+password from the UI once we are authenticated?

[m7pr]

In case some objects' names starts with .?

Suggested change

	if (identical(ls(data@env), character(0))) {
	if (identical(ls(data@env, all.names = TRUE), character(0))) {

[m7pr]

Suggested change

	#' and creating `teal_data` object. Such `server` function could look like this:
	#' and the creation of `teal_data` object. Such `server` function could look like this:

[m7pr]

I don't see how this can give any more control. I don't see examples a way of using it differently than with input and input_mask. Any chance we can provide advanced examples (maybe in a separate manual page) to reduce the clutter?

Suggested change

	#' If `ddl` developer values more control, then might be interested in using `...` explicitly,
	#' If `ddl` developers values more control, they might be interested in using `...` explicitly,

[m7pr]

Hey, I like the approach as it is easy to follow from the implementation perspective. I think it was originally created for tm_teal_data to be a part of modules, as it follow their structure but now it ended up in data argument as this creates teal_data object. I reckon it's better if this is passed to data as this one has a different purpose than any other modules.

(I do have a few implementation comments but I will hold off for now to first agree on the approach)

I do agree with @pawelru as I see we could provide similar comments from other approaches in here (example is the way we use regexes for input substitution).

eval_and_mask and .mask_code are easy to follow and exposed in server code of tm_teal_data which allows a user to also understand the implementation, as this is not hidded as in previous ddl_run examples. I also think the naming is better in this case.

I would only double check the name: tm_teal_data. tm in the names is already an acronim for teal module, so I understand currently tm_teal_data means teal_module_teal_data? What about just teal_data_module the same as the class name?

[m7pr]

just in case i think those also should be (tried to be) masked

[kartikeyakirar]

Given that tm_teal_data is accepted in the data argument, I prefer this approach over #957. However, I am curious whether it will also be accepted in both the modules and data arguments. Since tm_teal_data is a module, it might confuse users if it's not accepted in the modules argument.

[gogonzo]

I would even suggest to go step further with simplifying inputs and allow a module (i.e. list with ui and srv elements)

@pawelru exactly. But I would add also optional server_args to be able to pass some values to the server. I thought the same, if somebody writes a "ddl" as a simple module, then everything would be in place.

connector_xyz <- function(x) { 

  list(
    ui = function(id) { ... }
    server = function(id) {
      ...
      use(x)
      ...
    }
  )
}

In above example everything is fine, x is in the same environment where server is created. What if this is a package and server is created outside of this function?

I think that the PR #957 (transform module) should be an extension of this

I think it's exactly the opposite which I've proved here

---

This is why I think teal_module or any variation of this concept is the most robust and still can be simple. With teal_module one can specify only ui and server, but also one can pass any objects to the server through server_args. This is why I prefer this

[pawelru]

I think that the PR #957 (transform module) should be an extension of this

I think it's exactly the opposite which I've proved here

This is why I think teal_module or any variation of this concept is the most robust and still can be simple. With teal_module one can specify only ui and server, but also one can pass any objects to the server through server_args. This is why I prefer this

Hmmm... I was expecting this idea (i.e. module) to evolve into a simple module rather than the "transform module". We were thinking about the same but different naming :)
My understanding of "transform module" concept is roughly this: init(data = teal_data(...) |> teal_data2module_data(...), ...) - that is: modify teal_data further with additional logic in the module. Here we are talking more about init(data = module_data(...), ... directly where all the logic is within a module.

[gogonzo]

I was expecting this idea (i.e. module) to evolve into a simple module rather than the "transform module". We were thinking about the same but different naming :)

I agree, in teal::init(data = ) should accept teal_data or list(ui, server). Everything else would be just an extension of this. Basically, hieraarchy looks like this:

1. list(ui, server) (returning reactive teal_data)
2. (1) + attrs.
3. ddl(code, input_mask, ui, server) is an implementation of (2) with code and input_mask as attributes passed to the server.

So what I did in my last PR - I've changed assertion to accept data as a list(ui, server) and in server_teal_with_splash I'm adding additional attrs (if exist) to the server evaluation.

[gogonzo]

Closing this PR as this entirely covers this case. Alternative PR uses similar concept like module() but it doesn't need label, datanames. Alternative approach addresses also comments that object should be just a list(ui, server) which isn't the case with teal_module (ui, server, ui_args, server_args, label, datanames). Let's continue discussion there