-
Notifications
You must be signed in to change notification settings - Fork 117
Add Session abstract base class
#1331
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
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:
Sessionclass toAppSession.Session, from whichAppSession,SessionProxy, andExpressMockSesssionare subclasses of.Session.is_real_session(), which returnsTrueforAppSessionandSessionProxys made from anAppSession, and returnsFalseforExpressMockSessionandSessionProxys made from one of those.The new
Sessionclass 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
Sessionobject can remain unchanged. In most cases, when code wants aSessionobject, it can work with anAppSession,SessionProxy, orExpressMockSession.AppSessionrepresents a real, instantiated Shiny session for an appExpressMockSessionrepresents 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.SessionProxyrepresents a Shiny module session, where the parent aSession. The parent may be anAppSessionorExpressMockSession, or anotherSessionProxy.All of these must implement the various methods defined in
Session.One of the useful things about this is that
Sessionclass makes it very clear which attributes and methods are part of the interface we want to expose inAppSessionandSessionProxy. TheAppSessioncontains 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.AppSessionclass should be moved toSession.