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

Nested components proposal #265

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

joshiggins
Copy link
Contributor

Was thinking about nested components and started with simple test (failing right now as only top 2 layers of components get initialised).

Template tags were not loaded in the test settings, wondering if that is intentional... however existing tests still passing with them loaded.

@joshiggins
Copy link
Contributor Author

Okay this turned out to be a pretty small change really, it works for multiple layers of nesting and is enough to pass the test in there.

@joshiggins joshiggins marked this pull request as ready for review August 26, 2021 22:07
@adamghill
Copy link
Owner

Wow, this is awesome! At first glance this seems relatively straight-forward, thanks for getting it working. I'll test more thoroughly in the next few days and get this merged in asap.

@adamghill adamghill self-assigned this Aug 27, 2021

# this doesn't do what you expect because resulting dom is scoped to
# this component and the parent component won't get morphed
self.parent.is_editing = True
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshiggins So, I tried to add an example and kind of stumbled at this point. Your code "works" so I could merge it in as is, but Unicorn doesn't handle child components in a very intuitive way (I don't think). I'd love to hear any ideas about how this might work better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put some ideas in #253, makes sense to hold this PR off for a bit since it really only solves naive use case where components are nested but otherwise independent.

@joshiggins joshiggins marked this pull request as draft September 16, 2021 14:00
@joshiggins joshiggins changed the title Multiple layers nested components Nested components proposal Sep 16, 2021
@joshiggins
Copy link
Contributor Author

Updated example with proposal how nested components should communicate with each other.

For parent to child, the template tag kwargs already provide a mechanism for passing a value downwards (but see point 2 below).

For child to parent, the template tag kwargs can be used to pass down a callback function, which can be called by the child at the time of a relevant event (e.g. to tell the parent a button was pressed in the child component).

In terms of how to make this happen....

1. Passing a method to the template tag

See the @callback decorator, it could live somewhere inside django_unicorn.components module.

2. Update component when template tag kwargs change

At the moment the resolved kwargs from the template tag are only used when a component class is first instantiated by UnicornView.create.

If some property of a parent component is being passed to a child through a template tag kwarg and its value changes, the child component won't be aware of the new value because it has already been initialised and obtained from the cache.

My thoughts on this are

  • The django.views.generic.base.View constructor already adds all the keyword arguments as instance attributes
  • Unicorn component API can say explicitly that "template tag kwargs will be set as instance attributes on the component"
  • When component is created for the first time, behaviour is same as it is already
  • When component is retrieved from cache, the resolved kwargs provided to UnicornView.create update the existing instance attributes

3. Rendering via message view

Initial ideas on how to render the nested component tree - regardless of which component the message view is processing a request for, after the actions have been handled we can figure out which is the highest ancestor component that has been changed and actually render that one.

If a higher up component is being rendered instead, the current one the message view request came for will be a decendent of it and so indrectly rendered again anyway.

I think this would be acceptable for the initial implementation and an area for optimisation in future.

@adamghill
Copy link
Owner

adamghill commented Sep 24, 2021

Thanks for the detailed proposal!

1

The callback idea for child->parent is interesting and provides a level of explictness I appreciate. Another approach might be a "hook" (similar to parent_rendered https://github.com/adamghill/django-unicorn/blob/master/django_unicorn/components/unicorn_view.py#L263-L267) that could be child_event which has event_name and value as arguments. That would remove the need to pass each callback in as kwargs and fits into the current approach a little bit more. The trade-off is that it's less explicit and inside of the parent's child_event method you would need to check the event name and do some logic as opposed to having small, focused functions for any event that fires. Another approach would be to dynamically handle any method call that starts with on_child_ (or some other convention).

2

This might be the tricky part? I cache the class creation as a performance optimization, but it creates a few issues. Have you looked into how easy that might be achieve? There is an argument when creating the view to skip the cache, so that might be an option if we had a way to know that an instance had a change.

3

This makes sense. I generate a hash for the rendered version of every component to know if anything has changed or if I should return a 304. I wonder if we could go up the tree looking for the last component which has a new hash and then return that rendered template along with some smarts about the which target DOM element it applies to?

@adamghill
Copy link
Owner

@joshiggins Just checking back in on whether you have any more thoughts about this or able to implement any of it. Thanks!

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.

None yet

2 participants