-
Notifications
You must be signed in to change notification settings - Fork 92
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 Session
abstract base class
#1331
Conversation
shiny/express/_mock_session.py
Outdated
|
||
# Application-level (not session-level) options that may be set via app_opts(). | ||
self.app_opts: AppOpts = {} | ||
|
||
def is_real_session(self) -> Literal[False]: |
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.
Regarding the name of this method, eventually we'll probably have a MockSession that's used for testing, including testing server code (so would "implement" a lot more than ExpressMockSession, I'd imagine). If that happens, is_real_session
becomes a bit muddy.
is_server_session
? is_user_session
?
Naming things is hard...
I think this is a good start. Doesn't need to be part of this PR but it strikes me that part of what makes the AppSession class feel bad is that it does a whole bunch of things that are not that related to each other, and it's all jammed together because we want them to be presented to the user in one |
I agree that at some point it would make sense to break it up into separate classes. |
This PR does the following:
Session
class toAppSession
.Session
, from whichAppSession
,SessionProxy
, andExpressMockSesssion
are subclasses of.Session.is_real_session()
, which returnsTrue
forAppSession
andSessionProxy
s made from anAppSession
, and returnsFalse
forExpressMockSession
andSessionProxy
s made from one of those.The new
Session
class is an abstract base class, but it may be more useful to think of it as an interface which the other classes must implement.Most existing code which takes a
Session
object can remain unchanged. In most cases, when code wants aSession
object, it can work with anAppSession
,SessionProxy
, orExpressMockSession
.AppSession
represents a real, instantiated Shiny session for an appExpressMockSession
represents the fake session object that is present when rendering the UI of a Shiny Express app. Many of the methods on this class should not be called in practice, and so they will raise an exception if called.SessionProxy
represents a Shiny module session, where the parent aSession
. The parent may be anAppSession
orExpressMockSession
, or anotherSessionProxy
.All of these must implement the various methods defined in
Session
.One of the useful things about this is that
Session
class makes it very clear which attributes and methods are part of the interface we want to expose inAppSession
andSessionProxy
. TheAppSession
contains many other attributes and methods, but they are implementation details. With the state of the code before this PR, it was not clear which methods were meant to be part of the external interface and which were meant to be internal.This is still a work in progress, and there are still some open questions about which methods should be part of the interface and which should not.
Notes:
.is_real_session()
isn't great, so I'm open to other ideas._outbound_message_queues
,_downloads
, and_is_hidden
._send_message
,_send_message_sync
,_process_ui
. So perhaps these should be renamed to remove the leading underscore.AppSession
class should be moved toSession
.