-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add teal app modifiers and deprecate init args #1440
base: main
Are you sure you want to change the base?
Conversation
Unit Tests Summary 1 files 27 suites 10m 42s ⏱️ Results for commit 63065f0. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit d4f5a06 ♻️ This comment has been updated with latest results. |
1fb5028
to
aa60c14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some room for improvement
ef65f87
to
0af8a67
Compare
R/init.R
Outdated
|
||
app$server <- function(input, output, session) { | ||
old_server(input, output, session) | ||
custom_server(input, output, session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be possible to add a moduleServer
custom_server(input, output, session) | |
if (all(c("input", "output", "session") %in% names(formals(custom_server)))) { | |
callModule(custom_server, "wrapper") | |
} else if ("id" %in% names(formals(custom_server))) { | |
custom_server("wrapper") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we deprecate the id
parameter, this level will be a non-shiny-module root and I think it will keep things simpler if we keep it that way.
Do you see value in allowing app developers to pass shiny module ui in header/footer and adding the server module here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not pushing for this. I just realised that we shouldn't call custom_server(input, output, session) but use `callModule(custom_server, "wrapper") instead to separate possible namespace collision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There won't be any namespace collision as ui_teal
/srv_teal
is shiny module with a "teal" namespace and the root level of shiny session namespace is free for the taking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if inside of the custom server one calls a module with the namespace teal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedhav ⬆️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if inside of the custom server one calls a module with the namespace teal
@vedhav Sorry I didn't use question mark in the above comment. I hope we continue to discuss this.
There won't be any namespace collision as ui_teal/srv_teal is shiny module with a "teal" namespace and the root level of shiny session namespace is free for the taking.
Yes, there will be namespace collision if somebody calls twice add_custom_server
where some ids
are duplicated. There will be a collision because if we don't callModule
both modules would operate under teal
namespace.
My suggestion though set another trap - if one uses add_custom_server
twice, then both will be called with an id ="wrapper"
and we'll have a collision.
P.S. you've committed my two suggestions where assert_function(custom_server, args = c("input", "output", "session")
makes if ("id" %in% names(formals(...)))
impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the suggestion and added an example to call the module server. Please have a look tomorrow!
Co-authored-by: Dawid Kałędkowski <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
R/TealAppDriver.R
Outdated
app <- app |> | ||
add_landing_popup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To introduce the pipe operator in the internal code we need to agree to certain style rules within a team. Pipe might be overused to replace a single call which in my opinion is bad as it doesn't bring clarity plus it is a extra sugar to evaluate.
app <- app |> | |
add_landing_popup( | |
app <- add_landing_popup( | |
app, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ui_teal
shouldn't have header
, footer
, title
arguments. Deprecation in init only is a half-measure. Please use add_header
and add_title
in teal::init
body instead of passing arguments to ui_teal
.
Edit: Same applies to module_teal_with_splash
R/init.R
Outdated
details = "Use `modify_title()` on the teal app object instead." | ||
) | ||
} else { | ||
title <- build_app_title() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If build_app_title
going to be removed, maybe use the substitution for build_app_title
in here? Or it's going to stay, but will just be unexported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence here. I don't mind removing it completely or stop exporting it. I lean towards completely removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an exported function. We should soft_deprecated it first. It should be also taken away from default ui_teal(title) and ui_teal_with_splash(title).
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: Vedha Viyash <[email protected]>
Hey, really great start! A food for thought below
|
Closes #1310 and #1143
Changes
title
,header
, andfooter
are soft deprecated in favor of usingadd_title
,add_header
,add_footer
, andadd_landing_popup
add_custom_server
modifier to add custom server logic to the main shiny app.landing_popup_module
to the previous way by passing it inside themodules
|>
Example app with the new app modifiers.