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

Allow declaring methods and publications under models #1820

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ebroder
Copy link
Member

@ebroder ebroder commented Sep 8, 2023

@zarvox - I know you've expressed concern in the past about how TypedMethod and TypedPublication were leading to a very flat code structure, so I decided to see if I could come up with a better alternative. I'm not entirely sure if I think this is an improvement, but I think it does have some nice properties.

Almost all of our publications and models are associated (or at least primarily associated) with a model. This is reflected in the way we name our methods ("verbNoun"). However, we previously placed all methods in a single flat directory and did the same with publications.

Instead, allow declaring methods and publications as part of creating a new model class. This has the convenient side-effect of reducing the imports you need to operate on a model to just the model class itself. It also makes the internal identifier for a method or publication match how we refer to it in code (e.g. ChatMessages.methods.send).

For now, the implementations of the methods and publications are still in the same files as before; that's potentially something to reevaluate in the future. Additionally, it arguably makes it harder to enumerate all methods and publications, but that might be an acceptable tradeoff.

Almost all of our publications and models are associated (or at least
primarily associated) with a model. This is reflected in the way we name
our methods ("verbNoun"). However, we previously placed all methods in a
single flat directory and did the same with publications.

Instead, allow declaring methods and publications as part of creating a
new model class. This has the convenient side-effect of reducing the
imports you need to operate on a model to just the model class itself.
It also makes the internal identifier for a method or publication match
how we refer to it in code (e.g. `ChatMessages.methods.send`).

For now, the implementations of the methods and publications are still
in the same files as before; that's potentially something to reevaluate
in the future. Additionally, it arguably makes it harder to enumerate
all methods and publications, but that might be an acceptable tradeoff.
@ebroder ebroder requested a review from zarvox September 8, 2023 17:41
@ebroder
Copy link
Member Author

ebroder commented Sep 8, 2023

Oh another note: we can probably find a model to house all of our methods/publications under, but it's not always going to be clean. E.g. teamName and mediasoup:debug are both kind of messy. We can either leave those as flat-declarations or jam them under a model (e.g. Settings or try and do a better job of materializing the TeamName model for the former; the latter is...tricky).

Methods tend to be a little cleaner, but e.g. some of the setup page methods are tricky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant