Prevent too early evaluation

[gogonzo]

When server_args contains element being a language then it is being evaluated by do.call. We call do.call when calling modules in teal. Adding quote = TRUE to the call fixes the problem. See an example app:

tm_query <- function(query) {
  module(
    ui = function(id) {
      ns <- NS(id)
      verbatimTextOutput(ns("out"))
    },
    server = function(id, data, query) {
      moduleServer(id, function(input, output, session) {
        output$out <- renderPrint({
          q <- eval_code(data(), query)
          print(q)
        })
      })
    },
    server_args = list(query = query)
  )
}


library(teal.code)
library(teal.data)
pkgload::load_all("teal")

app <- init(
  data = teal_data() |> within(a <- iris),
  modules = modules(
    tm_query(query = quote(a_filtered <- subset(a, a$Species == "setosa")))
  )
)

runApp(app)

Alternative is to ignore this error and replace quote() with expression() as expression is not evaluated in do.call.

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  67      11  83.58%   41, 43, 85-93
R/get_rcode_utils.R                  12       0  100.00%
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                             99      42  57.58%   150-159, 161, 173-194, 219-222, 229-235, 238-239, 241
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     127  19.62%   47-68, 88-138, 143-144, 156, 203, 238-315
R/module_data_summary.R             203      37  81.77%   26-54, 68, 78, 232, 263-267
R/module_filter_data.R               64       2  96.88%   22-23
R/module_filter_manager.R           230      57  75.22%   56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R                 74       0  100.00%
R/module_nested_tabs.R              226      85  62.39%   40-136, 168, 193-195, 312, 344
R/module_snapshot_manager.R         216     146  32.41%   89-95, 104-113, 121-133, 152-153, 170-180, 184-199, 201-208, 215-230, 234-238, 240-246, 249-262, 265-273, 303-317, 320-331, 334-340, 354
R/module_teal_data.R                149      76  48.99%   41-144
R/module_teal_lockfile.R            131      44  66.41%   32-36, 44-56, 59-61, 75, 85-87, 99-101, 109-118, 121, 123, 125-126, 160-161
R/module_teal_with_splash.R          12      12  0.00%    22-38
R/module_teal.R                     195      87  55.38%   48-143, 158, 184-185, 224
R/module_transform_data.R           110       4  96.36%   20, 59, 129-130
R/modules.R                         278      71  74.46%   171-175, 230-233, 354-374, 382, 532-538, 551-559, 574-622, 655, 667-675
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 24      24  0.00%    17-42
R/tdata.R                            14      14  0.00%    19-61
R/teal_data_module-eval_code.R       24       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 20       0  100.00%
R/teal_data_utils.R                  10       0  100.00%
R/teal_reporter.R                    68       6  91.18%   69, 77, 125-126, 129, 146
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/teal_transform_module.R            45       0  100.00%
R/TealAppDriver.R                   353     353  0.00%    52-735
R/utils.R                           253      38  84.98%   405-454
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              15      11  26.67%   4-18
TOTAL                              3329    1328  60.11%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 9344594

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   27 suites   8m 59s ⏱️
275 tests 271 ✅ 4 💤 0 ❌
536 runs  532 ✅ 4 💤 0 ❌

Results for commit 9344594.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-data_summary	💚	$50.92$	$-1.77$	$0$	$0$	$0$	$0$
shinytest2-filter_panel	💚	$42.44$	$-1.54$	$0$	$0$	$0$	$0$
shinytest2-init	💚	$27.26$	$-1.37$	$0$	$0$	$0$	$0$
shinytest2-landing_popup	💚	$44.87$	$-2.00$	$0$	$0$	$0$	$0$
shinytest2-module_bookmark_manager	💚	$36.24$	$-2.39$	$0$	$0$	$0$	$0$
shinytest2-modules	💚	$39.09$	$-1.62$	$0$	$0$	$0$	$0$
shinytest2-reporter	💚	$68.21$	$-2.94$	$0$	$0$	$0$	$0$
shinytest2-teal_data_module	💚	$48.52$	$-1.58$	$0$	$0$	$0$	$0$

Results for commit 907dd40

♻️ This comment has been updated with latest results.

[llrs-roche]

I'm not that familiar with expressions, calls and languages, this seems a simple fix to the issue. But I leave the review for other that might understand better the implications of this change.

I noticed that callModule is "soft-deprecated":

Note: As of Shiny 1.5.0, we recommend using moduleServer() instead of callModule(), because the syntax is a little easier to understand, and modules created with moduleServer can be tested with testServer().

I don't think this would help on line 344, but perhaps is something to keep in mind.

[averissimo]

edit: nevermind about this first sentence, rethinking it doesn't make sense for quoted arguments I think this responsibility is on the module developer. See example below that works in main with only 1 line changing (query <- enquote(query)

I don't see any bad consequences on this change, the PR is fine and I tested with different types of native language objects.

I left a comment about adding a test for this so we don't risk a regression in the future.

pkgload::load_all("teal", export_all = FALSE)
tm_query <- function(query) {
  query <- enquote(query)
  module(
    ui = function(id) {
      ns <- NS(id)
      shiny::tagList(
        verbatimTextOutput(ns("out")),
        verbatimTextOutput(ns("out2"))
      )
    },
    server = function(id, data, query, yada) {
      moduleServer(id, function(input, output, session) {
        output$out <- renderPrint({
          q <- eval_code(data(), query)
          print(q)
        })
      })
    },
    server_args = list(query = query)
  )
}


library(teal.code)
library(teal.data)

app <- init(
  data = teal_data() |> within(a <- iris),
  modules = modules(
    tm_query(query = quote(a_filtered <- subset(a, a$Species == "setosa")))
  )
)

runApp(app)

[averissimo]

Consider adding a test before merging, such as:

testthat::it("srv_teal_module.teal_module accepts quoted arguments", {
  tm_query <- function(query) {
    module(
      "module_1", 
      server = function(id, data, query) {
        moduleServer(id, function(input, output, session) {
          reactive(q <- eval_code(data(), query))
        )
      },
      server_args = list(query = query)
    )
  }
  shiny::testServer(
    app = srv_teal,
    args = list(
      id = "test",
      data = teal.data::teal_data(a_dataset = iris),
      modules = modules(tm_query(quote(a_dataset <- subset(a_dataset, Species == "setosa"))))
    ),
    expr = {
      session$setInputs(`teal_modules-active_tab` = "module_1")
      session$flushReact()
      
      testthat::expect_setequal(
        "setosa",
        unique(modules_output$module_1()()[["a_dataset"]]$Species)
      )
    }
  )
})