EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
953 stars 59 forks source link

feat: slot context resolution + slot fill mgmt refactor #437

Closed JuroOravec closed 1 month ago

JuroOravec commented 1 month ago

First stab at https://github.com/EmilStenstrom/django-components/issues/350.

Unlike our discussion, I made the slot resolution configurable via "slot_context_behavior" key, with the options below:

The reason why I went for configurable behavior is that 1) What we agreed on in https://github.com/EmilStenstrom/django-components/issues/350 was the prefer_root option, 2) I don't remember the exact link, but when at one point I was going through older discussions in this project, I got a feeling that there was 1-2 tickets for changing the slot behavior akin to prefer_root / isolated, and one for allow_override. 3) Since I was implementing this, and the idea of supporting multiple "modes" was on the table, I'd like to have the option to have complete isolation, hence the isolated option.

I've included description and examples of each type in the settings.

I also did a bit of internal refactoring. See https://github.com/EmilStenstrom/django-components/pull/437/commits/2a8005c4d4fb16ae92679ad6997d0363e3ba37fe and https://github.com/EmilStenstrom/django-components/pull/437/commits/696e5c48d1a24a5e526e282e7b28dea015cd440e.

I also refactored the FillNode classes, merging ImplicitFillNode, NamedFillNode and BaseFillNode. Because instead of ImplicitFillNode, we could simply have a NamedFillNode with is_implicit: True (See https://github.com/EmilStenstrom/django-components/pull/437/commits/c66437792225058407a48146d7747845303f9205)

TODO:

UPDATE 2024-04-16

Ooof, the refactor got out of hand 😅

  1. I had troubles debugging the flow, so I refactored how we manage the slot fills
    • Previously, we had a ChainMap under the "_DJANGO_COMPONENTS_FILLED_SLOTS" key of the Context. I had 2 issues with this:
      • Django's Context object is already a stack, meaning we can pop/push layers. But also ChainMap was a stack. It wasn't easy for me to understand how to update the ChainMap in relation to the Context (AKA when to pop/push what).
        • So to simplify it, I removed the ChainMap, and instead all slot fills now live directly on the Context stack. So now there is only one object that manages the stacks (Context).
      • Slot fills stored on the ChainMap were using the Template instance as dictionary keys. My problem with this was that it was practically impossible to make sense of it using terminal. Also, my understanding is that the Template instances are cached/reused by Django. Yet, in the code we modified the Nodes in the Template.nodelists. So on one hand we used something that's supposed to be immutable (dict key), yet there were some parts of it that we DID change (underlying Nodes). So I wanted to simplify this, so it's clear when something IS or IS NOT immutable.
        • So instead of dict keys, we now:
          1. Create unique IDs for the Nodes we create (ComponentNode, SlotNode, FillNode)
          2. We use and pass through the component's ID (component_id) to tie together the components, fill nodes and slots.
          3. We use the component_id instead of the Template instance in the dicts.
  2. During the refactor, at one point everything was working, except for looping logic. I had hard time troubleshooting this, so I've inserted log statements to almost all important parts of the flow, to be able to go through the logs step-by-step.

~What's still remaining is to update the README. Otherwise, both tests and the sampleproject are passing on my end.~

Here's example how the logs looks like with the tracing logs and node IDs:

Screenshot 2024-04-16 at 15 28 43
JuroOravec commented 1 month ago

@EmilStenstrom @dylanjcastillo @lemontheme, since this MR got much larger than anticipated, I'd love if each of you could have a look at this and share your thoughts when you get the time, thanks!

JuroOravec commented 1 month ago

Thanks again, really appeciate the feedback from both of you so far @EmilStenstrom @dylanjcastillo!

I rebased, and fixed/resolved the small points. I'll come back to the point on trace logging tomorrow/Thu.

dylanjcastillo commented 1 month ago

I just approved it!

EmilStenstrom commented 1 month ago

@JuroOravec Wanna merge and do a release? :)

JuroOravec commented 1 month ago

Thanks both, merged!

@EmilStenstrom Here's MR for the release https://github.com/EmilStenstrom/django-components/pull/442. Not sure if I can push tags. Or can I use the github UI for that?


Update - I used the UI, looks like it also created a Release entry.

EmilStenstrom commented 1 month ago

Awesome!

JuroOravec commented 1 month ago

Just a follow up on the release - I just realized that I was making the MR from my fork. That's why it didn't work when I tried to push tags - they were pushed, but to my fork 😅