search

EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
942 stars 57 forks source link

Remove options for slot context behavior for v1.0 #450

Closed JuroOravec closed 4 days ago

JuroOravec commented 1 week ago

This is a follow up from https://github.com/EmilStenstrom/django-components/pull/445#issuecomment-2073079048.

For context, slot context settings is about how the variables inside the slot are evaluated. For more info see https://github.com/EmilStenstrom/django-components/issues/350

@EmilStenstrom suggested to have a single behavior for slot context, instead of having settings that allow you to configure how it works. (See https://github.com/EmilStenstrom/django-components/pull/445#pullrequestreview-2017902791)

As for me, my motivation to introduce the feature was to be able to use the isolated setting for my project (I prefer the isolation as it mimics behavior of Vue or React). I can also imagine that some people may want to set the config to allow_override to keep the behavior as it was before v0.67. So IMO we should provide backward compatibility until v1.0.

Thoughts?

EmilStenstrom commented 1 week ago

I'd love to hear other thoughts here.

My thoughts: Removing options is never something that a community will vote for, it's something that you do to remove complexity, both from the code and for users. For users that means fewer ways to use the system, which means easier troubleshooting.

So long story short: I think removing the other options might make this project easier to maintain. It's not a strong opinion though.

dylanjcastillo commented 1 week ago

I guess people would be more used to the isolated setting given that it’s the default option in Vue/React. I also wonder how many people are consciously aware of how allow_override works. IMO feels like a behavior prone to errors.

Maybe we could change the isolated to default, add a deprecation warning for the other two until 1.0 and, if there’s no push back, deprecate them?

JuroOravec commented 1 week ago

@dylanjcastillo Yeah, I'm thinking the same.

Also, as I was fixing things around in the last few days, I also got a bit of experience from user's perspective, which leads me to think that, if context_behavior settings was to remain, then:

  1. context_behavior and slot_context_behavior should be merged.

    Originally I added slot_context_behavior separately, as a standalone feature. But from user's perspective, configuring these two as separate values feels like unnecessarily too much detail.

  2. The prefer_root option of slot_context_behavior should be dropped.

    After all, the prefer_root is very close in behavior to isolated.

  3. The context_behavior options could be renamed to isolated and django - django meaning that the templates behave like in regular django templates.

EmilStenstrom commented 1 week ago

@JuroOravec So one option context_behavior, with two options, isolated and django, with isolated being the default. That would work for me.

Do we really need the "django" option then? If we believe isolated is safer, whey not go all in and have that as the only option?

JuroOravec commented 1 week ago

@EmilStenstrom I've been around the project briefly, so don't know the history, but there is a suite of about 5-10 tests using various combinations of the features of the django mode. E.g. one of the latest changes on that was https://github.com/EmilStenstrom/django-components/issues/272 and https://github.com/EmilStenstrom/django-components/pull/273. So I assume some people were/are using it?

I prefer the isolated approach (Vue-like), but that's just my opinion. The django settings is what has been in effect before for who knows how long, so there may be a lot of people who built projects on django-components using the native nesting behaviors of Django templates. For them, using only the isolated settings could be a fairly radical change (requiring a lot of refactoring), which could potentially discourage them from upgrading.

The good news tho, is that at least for us maintaining the "django" and "isolated" settings should be easier now. I took a deep dive and rewrote the slot resolution logic, because I got to a stage where some tests were always failing and I couldn't figure out why. And in that rewrite the difference between using the "django" or "isolated" modes is minimal.

dylanjcastillo commented 1 week ago

I think keeping both makes sense, especially if the difference is small.

OTOH, I think we should do this change before 1.0. Otherwise, making changes might be unnecessarily complex (and might discourage contributors) given that we need to take into account some options that we won’t be supporting very soon. Maybe apply this change and provide a warning in cases when one of the deprecated values is provided?

EmilStenstrom commented 1 week ago

Good point about old projects likely using this. So then, I guess we would also break those projects by changing the default? Since they likely don't have this defined...

Agreed that this is something we should change before 1.0. Overall, let's prioritize things that break stuff, so that we have less of that after 1.0. I see the 1.0 milestone tag is on this issue, good!

JuroOravec commented 5 days ago

I've implemented the changes in https://github.com/EmilStenstrom/django-components/pull/465, @EmilStenstrom @dylanjcastillo please give it a look whe you've got the time :)

JuroOravec commented 4 days ago

Released as part of 0.70 🎉