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

Add Session abstract base class #1331

Merged
merged 22 commits into from
Apr 26, 2024
Merged

Add Session abstract base class #1331

merged 22 commits into from
Apr 26, 2024

Conversation

wch
Copy link
Collaborator

@wch wch commented Apr 23, 2024

This PR does the following:

  • Renames the existing Session class to AppSession.
  • Adds a new abstract base class named Session, from which AppSession, SessionProxy, and ExpressMockSesssion are subclasses of.
  • Adds a new method, Session.is_real_session(), which returns True for AppSession and SessionProxys made from an AppSession, and returns False for ExpressMockSession and SessionProxys 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 a Session object, it can work with an AppSession, SessionProxy, or ExpressMockSession.

  • An AppSession represents a real, instantiated Shiny session for an app
  • An ExpressMockSession 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.
  • A SessionProxy represents a Shiny module session, where the parent a Session. The parent may be an AppSession or ExpressMockSession, or another SessionProxy.

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 in AppSession and SessionProxy. The AppSession 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:

  • The name .is_real_session() isn't great, so I'm open to other ideas.
  • There are a number of properties which seem like they shouldn't be part of the interface, but they are. These include _outbound_message_queues, _downloads, and _is_hidden.
  • There are a number of methods which have a leading underscore, which makes it seem like they should be internal -- but in practice I found that they needed to be added to the new base class because they are somewhat external-facing. For example, _send_message, _send_message_sync, _process_ui. So perhaps these should be renamed to remove the leading underscore.
  • Docstrings from the AppSession class should be moved to Session.

@wch wch requested a review from jcheng5 April 24, 2024 12:45
@wch wch marked this pull request as ready for review April 24, 2024 12:45
@wch wch changed the title Add SessionABC class Add Session abstract base class Apr 24, 2024

# Application-level (not session-level) options that may be set via app_opts().
self.app_opts: AppOpts = {}

def is_real_session(self) -> Literal[False]:
Copy link
Collaborator

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...

@jcheng5
Copy link
Collaborator

jcheng5 commented Apr 24, 2024

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 session object. It would be nice to extract out the major pieces of functionality into separate classes, which the AppSession then instantiates and delegates.

@wch
Copy link
Collaborator Author

wch commented Apr 25, 2024

I agree that at some point it would make sense to break it up into separate classes.

@wch wch added this pull request to the merge queue Apr 26, 2024
Merged via the queue into main with commit 3547e20 Apr 26, 2024
32 checks passed
@wch wch deleted the session-abc branch April 26, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants