Nested children past the first level #507
Conversation
| I think this is a step in the right direction if it doesn't do all that is required. I'm still testing if it works, but I'm having issues getting the parent and grandchildren to update on the client side correctly so maybe there are js or clientside assumptions that need to be fixed as well. |
| Thanks for this initial step! I'll take a look in the next few days and let you know what I run into. :) |
| Sounds good. I think I've found a client side fix I'll add shortly (or init_js at least). I'm going to update an example too since that helps debug. I'm sure there'll be feed back to improve the example, but I wanted to get it working quick as a proof. |
💯 I have found that having both tests and examples is invaluable to make sure all the pieces work together as expected. I really appreciate all the work you are doing for this PR! Looking forward to seeing the JS portion and digging into the code as soon as I can. |
…re. Yet to do, send grandparent etc. changes to the client and update the client code to handle that.
…the client to read and parse that.
| I think I found the client changes needed. At least the example (with my ugly change) is working. I'll stop for now until you get a chance to take a look. I won't be surprised if I'll need to clean it all up some even if it is the right approach. :P |
| "component_id": self.component_id, | ||
| "component_name": self.component_name, | ||
| "component_key": self.component_key, | ||
| "component": self, |
There was a problem hiding this comment.
I'm relatively new to Django (in the last year) so I'm not too sure on how bad this would be to add to the context.
There was a problem hiding this comment.
I think this might be fine, but I do wonder if it exposes some data that shouldn't be available in the template. What data did you need in the template? If it was just parent and children, we could explicitly add them to the context? The other approach is add component like you have, but deprecate component_id, component_key, component_name since that would be duplicative going forward.
There was a problem hiding this comment.
I thought of deprecating those three component pieces, but figured it might break anyone dependent on them. If you're going to bump the minor version, it seems like a safer change to deprecate them.
On what I need component for: I'm using it to set the 'implied' parent. Basically I found that view was always only set to the highest level component, so it wouldn't work to pass in parent=view past the first level. Since this unicorn info is set in the context as it goes down the tree, it was the easiest way to get the parent to each child at each level. I could have used component_id/key/name to look up the parent again, but that seems inefficient when the parent was just called before the child.
It does expose the whole component to the template when all I really need it for is to set the parent of each child. However I didn't think of a cleaner way that wouldn't add look ups or complications.
| calls = [] | ||
| | ||
| def __init__(self, **kwargs): | ||
| # set all these as instance variables instead of just class variables. Otherwise, calling UnicornView() |
There was a problem hiding this comment.
I'm not sure if this was really required, but my tests were awkward without it. They'd have to call construct_component(<args>) instead of just UnicornView(<args>) but I assume in non-test it was working right.
There was a problem hiding this comment.
I'm pretty sure most of these class variables should be moved into the init, especially the ones that use {} because otherwise all instances will share the same dictionary which I'm pretty sure is not intended. Good catch.
There was a problem hiding this comment.
I did originally remove them from the class variables, but there is an odd check in Django views that says you can't set kwargs for a view that don't match a class variable: https://github.com/django/django/blob/main/django/views/generic/base.py#L89-L94 so I left the class variables but set the instance ones to override them.
| def get_cacheable_component( | ||
| component: "django_unicorn.views.UnicornView", | ||
| ) -> "django_unicorn.views.UnicornView": | ||
| class CacheableComponent: |
There was a problem hiding this comment.
I'll be interested in what you think of this approach. I found having to create the cacheable component, then restore back the extra_context/request everywhere was scary. Meaning I could forget a spot and not notice. I would hope the enter/exit/with approach would help with that, but maybe its awkward.
There was a problem hiding this comment.
This is a much better pattern and using a context manager totally makes sense for this use case.
| From my initial testing with the example this seems to be working as intended! 🤩 This is such an awesome PR, thank you again for all your work on this. I'm going to dig into the tests next and make that change to the class variables I mentioned in a comment above, but overall your code additions look really good. One other note: I will probably bump to the next minor version (not just a patch) because this touches so many different parts of |
Awesome! Glad its holding up to testing besides my own 😄. Let me know if I can make any changes or add anything to help it along. The minor version bump makes sense to me. I'd be interested to know around how long you think until the next release. I know this is a side project so I understand the answer may be something like "when I can find the time", but just checking in case you have an idea. As soon as it lands I'll start using it since I'm already using my fork for now to unblock a use case I have. |
As long as I don't run into any gotchas or concerns, I'll publish a new release tonight. 👍 |
| @all-contributors add @bazubii for code and tests |
| I've put up a pull request to add @bazubii! 🎉 |
| @bazubii 0.50.0 has been published to |
Awesome! I'll try it out and let you know if anything comes up. |
| Found one small thing so far. |
| Also found that the children aren't being set correctly for a parent. These lines: https://github.com/adamghill/django-unicorn/blob/main/django_unicorn/components/unicorn_view.py#L366-L375 The |
Actually I think its more around the caching, still investigating... |
| So I think this PR fixes the caching: #511 The behavior I was seeing was that components retrieved from the cache had partial children set. They were cached while rendering or during init, so the first child would have a parent with only one child, the second would have a parent with two children, etc. I'm going to have to test it more in my app as well, but I think that PR has a test to prove it works |
2 participants
Support nested children past the first level: