Skip to content
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

Revamp shiny modules #100

Merged
merged 13 commits into from Jun 30, 2022
Merged

Revamp shiny modules #100

merged 13 commits into from Jun 30, 2022

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Mar 2, 2022

This PR removes Module() in favor of @module.ui and @module.server decorators and also removes the need to explicitly namespace ids when generating UI. See examples/moduleapp/app.py for an example. There is one main downside to implicitly name-spacing in that binding developers will need to know that they need to wrap ids with module.resolve_id() (see, for example, the implementation of ui.input_action_button() or ui.output_text())

This also fixes the following:

Closes #99
Closes #167
Closes #103
Closes #102
Closes #101

TODO

  • Double-check we aren't missing any SessionProxy methods (i.e., Session methods that need to know about the namespace).
  • More tests

@cpsievert cpsievert mentioned this pull request Mar 2, 2022
4 tasks
shiny/modules.py Outdated Show resolved Hide resolved
shiny/modules.py Outdated Show resolved Hide resolved
shiny/session/_session.py Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented Mar 3, 2022

If possible, please add tests.

@cpsievert cpsievert changed the title Fix some ModuleSession namespacing issues Simplify and fix various issues with Module()s Mar 5, 2022
@cpsievert cpsievert force-pushed the ModuleSessionFixes branch 2 times, most recently from c51854b to 43f73f9 Compare March 7, 2022 16:20
@cpsievert cpsievert changed the title Simplify and fix various issues with Module()s Reduce need to explicitly namespace Module() UI code Mar 8, 2022
@cpsievert cpsievert force-pushed the ModuleSessionFixes branch 3 times, most recently from 6cc2a44 to 08115dd Compare May 18, 2022 16:40
shiny/_namespaces.py Outdated Show resolved Hide resolved
@cpsievert cpsievert force-pushed the ModuleSessionFixes branch 3 times, most recently from c921b8f to c1a2e83 Compare May 20, 2022 22:24
@cpsievert cpsievert changed the title Reduce need to explicitly namespace Module() UI code Revamp shiny modules May 20, 2022
@cpsievert cpsievert requested a review from jcheng5 June 1, 2022 22:22
@cpsievert cpsievert force-pushed the ModuleSessionFixes branch 2 times, most recently from 19c6c67 to 6b45957 Compare June 17, 2022 17:41
@jcheng5 jcheng5 self-assigned this Jun 18, 2022
@jcheng5
Copy link
Collaborator

jcheng5 commented Jun 18, 2022

To summarize where we are right now:

ui.output_plot("plot1")

In this example, "plot1" is a bare id.

No more ns("blah") calls all over your UI

If this code is invoked at a top-level (non-module) UI, a div#plot1 is generated. But if it's invoked in a module UI or module server function, say with module id "one", then a div#one-plot1 is generated. The operation that changes "plot1" to "one-plot1" is called "resolving the id against the current namespace context".

This isn't by magic; every UI function that takes an ID is going to need to call id = namespaced_id(id) or something like that.

Resolved id's use a special class

Resolving a string id "plot1" results in a ResolvedId object. The ResolvedId class is a subset of str. We use a distinct (sub)type so we can detect an attempt to resolve it against the current (or other) namespace; if such an attempt is detected, we just skip it. For example (pseudocode):

ns("foo") -> "one-foo"
ns(ResolvedId("foo")) -> "foo"

TODO

  • Change @shiny.module_ui and @shiny.module_server to @shiny.module.ui and @shiny.module.server.
  • Change namespaced_id() to shiny.module.resolve_id() or something like that.

shiny/_main.py Outdated Show resolved Hide resolved
shiny/session/_session.py Outdated Show resolved Hide resolved
shiny/session/_session.py Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented Jun 24, 2022

Some notes: We should have a test for auto-namespacing in dynamic UI, as well as a real app that exercises that.

Overall, the API looks good to me. The only thing I’m wondering about is if it’ll ever be an issue that input UI objects need to be created in the context of the module. In other words, is it possible for a module's UI object to be created outside of that context? The good thing is that this is less likely in Python than in R, due to the lack of lazy evaluation. That said, in R, we also introduced tagFunctions, which are intended specifically to not execute right away, and if we bring that concept to Python, we'd have to make sure they would execute with the correct namespace context.

@jcheng5
Copy link
Collaborator

jcheng5 commented Jun 24, 2022

@wch That's a really good point. If it helps, you can resolve an id as early as you want; once you've resolved it, it won't be resolved again.

def foo(id: Id):
  id = module.resolve_id(id)
  def _():
    slider_input(id)
  return _

When the _ is executed, the id will have already been resolved and further calls to resolve_id(id) won't affect it. Even if _ is invoked in the context of different module, it won't be resolved into some other namespace.

(Actually let me add a test for that)

@jcheng5 jcheng5 requested review from wch and removed request for jcheng5 June 25, 2022 03:51
@jcheng5
Copy link
Collaborator

jcheng5 commented Jun 25, 2022

I changed the example under examples/moduleapp/app.py to have two dynamic scenarios (one with just UI output, one with UI output wrapped in another module). Everything seems to work great.

@wch wch merged commit 1f9ad5d into main Jun 30, 2022
@wch wch deleted the ModuleSessionFixes branch June 30, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment