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

WIP: Named slots / Nested slots #1145

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

richard-to
Copy link
Collaborator

@richard-to richard-to commented Dec 10, 2024

I still need to add tests and maybe some more example test cases to make sure it covers all the edge cases correctly.

But before I do that, just wanted to make sure the API seems OK.

Changes

  • Adds support for multiple slots
  • Adds support for named slots.
  • Fixes issues with nested slots
    • Originally I had simpler fix for the nested slots issue (basically make node_slot as stack and then pop off of the top), but to support multiple and named slots, need a bigger change which also ended up covering the nested slot case

Issues

@wwwillchen
Copy link
Collaborator

Overall, I'm liking the direction of this. Thanks for working on this!

My main two suggestions are:

  1. I don't think we should support multiple unnamed slots. It seems: a) no real benefit compared to multiple named slots, b) harder to read the callsites (e.g. what's the difference between the first slot and second slot) and c) deviates from HTML slots: https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_templates_and_slots - based on my reading, in HTML, you can only have one unnamed slot, which acts like a default slot, and it takes in all the content implicitly as the slot (i.e. what our content_component currently does). I think keeping this similar to web API isn't totally necessary, but it's a good sanity check in terms of our API design. Interestingly, Angular itself models their concept (content projection) after the HTML standard, but with some differences to "angularize" it: https://angular.dev/guide/components/content-projection/

  2. This isn't 100% necessary for the feature, but ideally I'd like to have some form of stricter typing. For example, an ideal API for this would look something like this:

@me.content_component(named_slots=["header", "body"])
def comp():
  ...

def page():
  with comp() as c:
     with c.header():
       me.text("...")
     with c.body():
       me.text("...")

I'm not totally sure how to write this type annotation / implement this (maybe Gemini can help :), but I think this would provide a much nicer API for consumers for named slot content components because then you can get IDE support (e.g. autocomplete, type-checking)

@richard-to
Copy link
Collaborator Author

I think it's definitely doable to get this syntax, but does not seem like we'd be able to get the dynamic annotations/type hints in IDE in a nicer way.

def page():
  with comp() as c:
     with c.header():
       me.text("...")
     with c.body():
       me.text("...")

There is one option which is to use namedtuple

@me.content_component(named_slots=namedtuple('SomeName', ["header", "body"]))
def comp():

I think this would work, but I think doesn't feel like a good trade off. Since we'd need to create a different namedtuple for each set of named slots. Also wouldn't work if someone did: namedtuple('SomeName', slot_names_from_var)

Another option is for users to implement a dataclass and pass that into the named slots. But seems unnecessarily verbose and probably don't want to expose DetachedNodeTreeStateContext

@dataclass
class HeaderBodySlots:
  header: DetachedNodeTreeStateContext
  body: DetachedNodeTreeStateContext

@richard-to
Copy link
Collaborator Author

richard-to commented Dec 12, 2024

EDIT: Actually NVM, don't think that would work.

Hmm, one more idea that could work is to let the user pass in a Literal for the named slots like Literal['header', 'body']

@me.content_component(named_slots=Literal['header', 'body']))
def comp():

Drawback is that this would lead to an api like this:

def page():
  with comp() as c:
     with c.slot('header'):
       me.text("...")
     with c.slot('body'):
       me.text("...")

So not as a nice c.header, c.body

@wwwillchen
Copy link
Collaborator

EDIT: Actually NVM, don't think that would work.

Hmm, one more idea that could work is to let the user pass in a Literal for the named slots like Literal['header', 'body']


@me.content_component(named_slots=Literal['header', 'body']))

def comp():

Drawback is that this would lead to an api like this:


def page():

  with comp() as c:

     with c.slot('header'):

       me.text("...")

     with c.slot('body'):

       me.text("...")

So not as a nice c.header, c.body

I think the named slots w/ literals is a good idea. Not quite as ergonomic, but still readable and easy both for the content component producer and consumer.

@richard-to
Copy link
Collaborator Author

I think the named slots w/ literals is a good idea. Not quite as ergonomic, but still readable and easy both for the content component producer and consumer.

Unfortunately, I spoke too soon about using Literals. Don't think it will work. Was hoping something like this could work. But get `(Unknown) -> Any). Passing in a dynamic literal does work when not wrapped in the Callable. But unfortunately not too useful that way. So I'll just go ahead and implement the c.header(), c.footer()

from typing import Literal, Callable, NamedTuple, Any


def make_tuple(typeX):
  return NamedTuple("TestT", [("slot", Callable[[typeX], Any])])

TestT = make_tuple(Literal["header", "footer"])
 
t = TestT(slot=lambda a: a)
t.slot()

@wwwillchen
Copy link
Collaborator

I'd avoid namedtuple because it's not great API-wise for long-term maintenance compared to dataclass, see: https://snarky.ca/dont-use-named-tuples-in-new-apis/

Similar to how we do state class, what if we did this?

@me.slotclass
class ScaffoldSlots:
  header: me.NamedSlot
  body: me.NamedSlot
  footer: me.NamedSlot

@me.content_component(named_slots=ScaffoldSlots)
def ...

Essentially slotclass is an alias for dataclass, but we could do things like enforce [kw_only](https://docs.python.org/3/library/dataclasses.html#:~:text=in%20version%203.10.-,kw_only,-%3A%20If%20true%20(the).
me.NamedSlot is basically an alias to something like DetachedNodeTreeStateContext

Even though this is a bit more verbose than namedtuple, this should be pretty familiar to Mesop developers since it's quite similar to the stateclass syntax and it avoids the downsides of namedtuples (e.g. indexing).

@richard-to
Copy link
Collaborator Author

In terms of the namedtuple thing. I definitely agree about avoiding them. I was mainly using them since they apparently do have some typing support for dynamic creation use cases. Which I thought I could leverage with Literal to keep things less verbose.


Yeah, I did consider the dataclass option, but felt it would be too verbose. But I do like your version of it with the slotclass and the type alias for DetachedNodeTreeStateContext. Has a good symmetry and consistency to it.

I think that's more of a personal thing, having gotten used to working with python 2.7 and not have type hints most of the time. So not having the type hints doesn't bother me as much as too much boilerplate (I think that's one reason python initially got popular, because the syntax is relatively quick to write in comparison to say Java, but I think that was more a benefit in the past than in the present. The extra verbosity isn't that much of a problem these days with AI assistance to autocomplete boilerplate like this.

And I guess, if people were to make their custom components more shareable, it would be more user friendly for users to know what slots existed without having to look at the code (or docs if they exist)

Part of me wants to suggest making passing in the slotclass optional but I think that flexibility sort of muddles the API to have multiple ways of doing things.

So ok, I'll go with adding slotclass. Thanks for the brainstorming help.

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.

me.slot appending in nested content_components Support multiple slots
2 participants