teal reactivity solution 2 #806
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 04a81ad Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…_reactivity_v2@main
|
Looking at the log, it looks like module will only be processed when clicked. Can you try it on your end? |
Unit Test Performance DifferenceAdditional test case details
Results for commit 04a81ad ♻️ This comment has been updated with latest results. |
Could you be more specific with an example or a screenshot of the logs to compare? |
|
I tried it again and I don't see the behavior anymore, maybe my machine was too cluttered last night. Just for sanity check can someone else also test this out? |
| lapply(datanames, function(x) { | ||
| datasets$get_data(x, filtered = TRUE) | ||
| }) |
There was a problem hiding this comment.
I will measure timing of this and in the line 268: eventReactive(trigger_data(), datasets$get_data(x, filtered = TRUE))
I hope double evaluation of get_data() doesn't influence anything (filter operation suppose to be cached)
There was a problem hiding this comment.
I confirm that reactive in FilteredDataset$dataset_filtered is evaluated once and cached. This means that each datasets$get_data() takes just fractions of the seconds. So having multiple datasets$get_data() here doesn't have any influence on the performance.
There was a problem hiding this comment.
I prefer this solution over #804 as it doesn't need any additional changes. Also module_nested_tabs are less complicated as they don't need passing module_id to determine what is active. Apps works as expected. I give a pre-approval
| checkmate::assert_class(datasets, "FilteredData") | ||
| args <- isolate(teal.transform::resolve_delayed(modules$ui_args, datasets)) | ||
| args <- c(list(id = id), args) | ||
| args <- c(list(id = ns("module")), args) |
There was a problem hiding this comment.
line 102 - just checking that that's ok (for example missing_data and variable_browser have data in the UI)
|
It would be great if this doesn't trigger on initialization of the app only when the tab is created |
| @@ -223,7 +285,7 @@ testthat::test_that("srv_nested_tabs.teal_module doesn't pass filter_panel_api i | |||
| moduleServer(id, function(input, output, session) checkmate::assert_class(filter_panel_api, "FilterPanelAPI")) | |||
| }) | |||
|
|
|||
| testthat::expect_error( | |||
| testthat::expect_warning( | |||
There was a problem hiding this comment.
I could not find a better way to improve this test. It passes but throws an error internally.
The test does not really test what it claims it is testing.
There was a problem hiding this comment.
So I wonder if using rlang::env_has(environment(), "data", inherit = FALSE) (or datasets, or filterpanelapi) would help here? Or something similar?
There was a problem hiding this comment.
for the not passed case we want this:
moduleServer(id, function(input, output, session) {
checkmate::assert_false(
tryCatch(
checkmate::test_class(data, "tdata"),
error = function(cond) FALSE
)
)
})
```
and you replace data with datasets/filter_panel_api
for the it is passed case we can have assert_class() directly
and then expect_no_error (or expect_error(..., NA)) for the testserver
closes #803
Solution 2 to solve the reactivity of teal apps:
srv_nested_tabs.teal_moduleintomoduleServerrenderUIinsrv_nested_tabs.teal_moduleto trigger the filtered data when the tab is changed.datasets_to_datato accept areactiveValtrigger function to refresh the filtered dataPlease test with exploratory app or similar apps.