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

feat: dependency injection with inject/provide #506

Merged
merged 14 commits into from
Jun 1, 2024

Conversation

JuroOravec
Copy link
Collaborator

@JuroOravec JuroOravec commented May 23, 2024

This is a feature proposal for what I called "Global rendering context" in #483. The name might not have been the best, so I'll describe here what I had in mind / what I'm missing.

Background

I had a look at Django's RequestContext as mentioned in #483 (comment), but I don't feel like it's matching my needs. In my project there are cases where it doesn't make much sense to pass the Request instance to the rendering context, so relying on Django's context_processors feels more like a workaround than a proper solution.

Furthermore, I need to define different "globals" for different views (rendering runs). So again the approach with context_processors, which are defined globally in the settings, don't feel like a good fit.

To be clear, what I mean by "globals" is that when I have some template with nested components, then, in the "isolated" mode, the inner nested components can access these "globals" without the "globals" being passed via props. So actually, what I want to address with this is the issue of "prop drilling"

Since I'm dealing with prop drilling, I again took inspiration from Vue's inject/provide and React's Context/Provide.

The idea is that I can mark some variables to be "provided", and then any descendant of that component can access said variables by "injecting" it into it's context.

The easiest way to achieve this is leveraging Django's Context. However, it would be useful to access the "injected" variables in both get_context_data AND the component's template. But we don't have access to the Context inside get_context_data (which IMO is a good thing).

So that's how I arrived at the initial suggestion below.

class ParentComp(component.Component):
	provide = ["abc"]
	def get_context_data(self):
		return {
			"abc": 1,
		}

class ChildComp(component.Component):
	inject = ["abc"]
	def get_context_data(self, abc):
		return {}

How it works:

  1. A variable must be first "provided"
  2. To "provide" a variable in the parent component, we set the provide attribute to a list of variable names that we want to provide.
    • Names in the list MUST be in the dict returned from get_context_data, or an error is raised.
  3. To "inject" a variable in the child component, we set the inject attribute to a list of variable names that we want to inject.
    • Names in the list MUST be ALREADY provided by the ancestors. If a key is not found, an error is raised.
    • Variables are injected as kwargs, so get_context_data must specify the argument in function signature, or use **kwargs, or they get error.
    • NOTE: Injected variables can be defined on any ancestor. If multiple ancestors provide the same variable, then normal behavior of Django's Context applies, meaning that he variable is taken from the ancestor closest to the current component.
  4. An injected variable is passed to get_context_data, but is also automatically made available inside the component template, without having to explicitly expose the variable via get_context_data.

Example

class ParentComp(component.Component):
	provide = ["abc"]
	def get_context_data(self):
		return {
			"abc": 1,
		}
	template = """
		{% component "child" %}
		{% endcomponent %}
	"""

class ChildComp(component.Component):
	inject = ["abc"]
	def get_context_data(self, abc):
		print("injected abc: ", abc)  # <-- Injected var in get_context_data
		return {
			"xyz": 2,
		}
	template = """
		abc: {{ abc }} <-- Injected var in template
		xyz: {{ xyz }}
	"""

Further notes

  • In theory the provide/inject attributes could be skipped, and instead we could take an inspo from fastAPI and django-ninja, where they make use of inlined annotations to convey meaning. So the API would look like so:

     class ParentComp(component.Component):
     	def get_context_data(self):
     		return {
     			"abc": Provide(1),   # <-- wrapped in `Provide` means it will be provided
     			"zet": 1,                    # <-- NOT provided
     		}
     
     class ChildComp(component.Component):
     	def get_context_data(self, abc: Inject[int]): # <-- wrapped in `Inject` means it will be injected
     		return {}

    The upside of this approach that user specifies the variable name only once, whereas they have to specify it twice with the provide=["abc"] approach.

    However, I'm not yet familiar with how Python annotations like Inject[int] would work, so for simplicity's sake I'd go with the original approach for now.

TODO

  • Agree on API
  • Tests
  • README

@JuroOravec
Copy link
Collaborator Author

@EmilStenstrom @dylanjcastillo Let me know what you think of this one, thanks!

@dylanjcastillo
Copy link
Collaborator

Hey @EmilStenstrom @JuroOravec, a bit of an off-track comment:

Sorry, I've been absent lately.

I've been revising tasks and priorities for the next few months. I don't think I'll have time to contribute to django-components until the last quarter of the year.

Once things settle down, I'll try to engage more actively. Until then, I'll probably be a passive supporter of the project.

Thanks for understanding, and I look forward to getting back on track soon!

@EmilStenstrom
Copy link
Owner

@JuroOravec All this work and new concepts just because "isolated components are easier to maintain" :). I'm starting to doubt that all this work might be more work than having components break sometimes because the all use global scope...

BUT, we do support isolated, and I think we should continue supporting it well.

I think provide/inject looks complicated. The magic types version too. It looks like what we want is to "let a component switch to django context_behavior for some variables, and then have subcomponents explicitly declare they want those semi-global variables.

Trying to think of a concrete example where this would be useful (please correct me if I'm wrong):

{% component "table" rows=rows metadata=metadata %}
    {% for row in rows %}
        {% component "table_row" row=row %}
            {% for cell in row.cells %}
                {% component "table_cell" cell=cell %}
                    "{{ cell.data }}" from table {{ metadata.name }}
                {% endcomponent %}
            {% endfor %}
        {% endcomponent %}
    {% endfor %}
{% endcomponent %}

With context_behavior="isolated" this wouldn't work, we would have to add metadata to both the table_row and table_cell components. But if the "table" component could provide a metadata block, the cell could inject it?

@JuroOravec
Copy link
Collaborator Author

All this work and new concepts just because "isolated components are easier to maintain" :). I'm starting to doubt that all this work might be more work than having components break sometimes because the all use global scope...

Easier to maintain for the users, of course. Not necessarily the maintainers :)

Again, this is an advanced feature, which is useful for either more complex cases, or for component library authors like me.

It looks like what we want is to "let a component switch to django context_behavior for some variables, and then have subcomponents explicitly declare they want those semi-global variables.

Yes, that's a good way to think about it, that I explicitly opt-in into the "django" context_behavior.

Example 1 - Layouting

Where I felt the need was when I was working with "layout" components.

The project_layout component below defines a full HTML page, along with HTML for:

  • Side navigation at the top of the page (static, doesn't ever change)
  • Sidebar with left with a list of bookmarks (content changes with each page)
  • Breadcrumbs navigation (changes with each page)
  • Main content area that can be optionally split into 2 columns (changes with each page)

So I can use project_layout component to generate all HTML web pages, and all I need is to plug in the content
that I want to have rendered on the page.

project_layout.html
<!DOCTYPE html>
<html lang="en" class="h-full">
  <head>
	...
  </head>
  <body>
	<div class="header">
	  {% fill "header" %}
	    {% component "breadcrumbs" items=final_breadcrumbs %}{% endcomponent %}
	  {% endfill %}
	</div>

	<div class="sidebar">
	    {% component "bookmarks" bookmarks=final_bookmarks project_id=project.pk %}{% endcomponent %}
	</div>

	<div class="content">

		{% component "navbar" @sidebar_toggle="toggleSidebar" %}{% endcomponent %}

	    {% slot "header" %}{% endslot %}

	    <div class="flex">

	      {# Split the content to 2 columns, based on whether `left_panel` slot is filled #}
	      {% if component_vars.is_filled.left_panel %}
	        <div class="w-1/3 h-12">
	          {% slot "left_panel" %}{% endslot %}
	        </div>
	        <div class="w-2/3 h-12 pl-8">
	      {% endif %}
	
	      {% slot "content" default %}{% endslot %}
	
	      {% if component_vars.is_filled.left_panel %}
	        </div>
	      {% endif %}

	    </div>
	</div>
  </body>
</html>
project_layout.py
@component.register("project_layout")
class ProjectLayout(Component):
    template_name = "project_layout.html"

    def setup_context(
        self,
        *_args,
        project: Project,
        breadcrumbs: list[Breadcrumb] | None = None,
        bookmarks: list[Bookmark],
    ):
        final_breadcrumbs = process_breadcrumbs(breadcrumbs or [])
        final_bookmarks = process_bookmarks(bookmarks)

        return {
            "final_breadcrumbs": final_breadcrumbs,
            "final_bookmarks": bookmarks,
            "project": project,
        }

⚠️ But (!), as you can see in project_layout.py, the component needs some inputs too - project, breadcrumbs, and bookmarks.

The project_layout component is not the "top-level" component, and it's nested in another one*. So there's one or more components to which I need to pass the layout inputs (project, breadcrumbs, and bookmarks), altho they don't do anything with the data (example below).

* Note: In my project, there's actually 2-3 layers of "layout" components, as 1 defines the HTML base with <head> and <body>, while the other one(s) defines the page-specific layout.

project.html
{% component "project_layout"
  project=project
  bookmarks=bookmarks
  breadcrumbs=breadcrumbs
%}
  {% fill "header" %}
	...
  {% endfill %}

  {% fill "left_panel" %}
    ...
  {% endfill %}

  {% fill "content" %}
      ...
  {% endfill %}
{% endcomponent %}
project.py
@component.register("project")
class ProjectPage(Component):
    template_name = "project.html"

    def setup_context(
        self,
        *_args,
		# Redacted
        var1: list[Any],
        var2: list[Any],
        var3: dict[str, list[Any]],
        var4: list[Any],
        var5: list[Any],
        # Used by project layout
        bookmarks: list[Bookmark],
        project: Project,
        breadcrumbs: list[Any] | None = None,
        **kwargs,
    ):
		# pre-process here...

        return {
            "breadcrumbs": breadcrumbs or [],
            "bookmarks": bookmarks,
            "project": project,
            "tab_items": tab_items,
            "var1_processed": var1_processed,
            "var2_processed": var2_processed,
        }

Currently, I have about 10 components that use project_layout, and my issue is two-fold:

  • When I add or remove an input of project_layout, I need to update code in at least 32 places (1 in layout HTML, 1 in layout python file, 10 HTML files that use the project_layout component, 10 corresponding component Python files, and 10 views where I pass the input into the render function).

    Since there's no tooling for django-component for validating component input or variables used inside HTML, then, in most of these places, I get no linter help / no warning. So this is quite error prone.

    Now, with provide/inject, I could reduce this to 13 (1 in layout HTML, 1 in layout python file, and 10 views, 1 where I provide the data for the layout). I would basically skip one layer of components prop-drilling.

  • The project component shouldn't care about the input to the layout component. Because I have to prop-drill the data to the layout component, the "page" components (that use "layout" components) are bloated and harder to read, because I can't distinguish which inputs are actually relevant to the "page" component, and which ones are only passed through to the "layout" component.

Example 2 - Configuring or styling component library

This is a consideration far down the line, but with provide/inject, I could write UI component library that would allow users to have a fine-grained control over the components, while still providing a clean/terse interface.

This specifically is inspired by how Vuetify components work internally. And I guess React component libraries use the same principle with React's context provider.

Let's consider the example you wrote, but without the metadata input:

{% component "table" rows=rows %}
    {% for row in rows %}
        {% component "table_row" row=row %}
            {% for cell in row.cells %}
                {% component "table_cell" cell=cell %}
                    "{{ cell.data }}" from table
                {% endcomponent %}
            {% endfor %}
        {% endcomponent %}
    {% endfor %}
{% endcomponent %}

With the provide/inject feature, and taking inspiration from Vuetify, the snippet above could be condensed into:

{% component "ui_table" rows=rows %}
  {% component "ui_table_row" %}
    {% component "ui_table_cell" %}
		{% fill "cell" data="cell_data" %}
          "{{ cell_data|safe }}"
		{% endfill %}
    {% endcomponent %}
  {% endcomponent %}
{% endcomponent %}

What we would achieve in the example above is that we would render a table, but wrap all it's cell contents with double quotes, so "John Doe" instead of John Doe.

Note: I prefixed the component names with ui_ so it's clearer that they are part of a component library.

What happens above is the following:

  1. We pass data to ui_table.
  2. ui_table has a default slot that defines how a single row should be rendered.
  3. ui_table also "provides" data about the table data, and other, like styling
  4. Inside ui_table's row slot, we use ui_table_row component. ui_table_row internally injects the data provided by ui_table. So, as long as ui_table_row is defined INSIDE ui_table, it already has access to the data, and we don't need to pass it manually, nor manage looping.
  5. Same as step 3, but this time ui_table_row provides cell and styling data.
  6. Same as step 4, but this time it's ui_table_cell accessing data from ui_table_row.
  7. Because we want to keep all styling and scaffolding as is, and only want to override the cell content, we use ui_table_cell (which sets the scaffolding and styling). And we use the cell fill inside it to access the only the data for the given cell (remember that data="cell_data" is the "scoped slot" feature).

The beauty of this approach is that if we want to stylize the component, or otherwise configure it, we need to configure only the ui_table component. The nested ui_table_row and ui_table_cell would "inherit" the styling/config in the background.

E.g. in this example, the info about color="red" would be internally propagated to ui_table_row and ui_table_cell.

{% component "ui_table" rows=rows color="red" %}
  {% component "ui_table_row" %}
    {% component "ui_table_cell" %}
		{% fill "cell" data="cell_data" %}
          "{{ cell_data|safe }}"
		{% endfill %}
    {% endcomponent %}
  {% endcomponent %}
{% endcomponent %}

One could argue that "sure, but we could do the same with Django settings". But if we used a singular place for defining component styling, then all the instances of ui_table would look the same. On the other hand, with the approach above, we can make changes per component.

E.g. below, we have two tables, one which would be in red, and one in blue:

{% component "ui_table" rows=rows color="red" %}
  {% component "ui_table_row" %}
    {% component "ui_table_cell" %}
		{% fill "cell" data="cell_data" %}
          "{{ cell_data|safe }}"
		{% endfill %}
    {% endcomponent %}
  {% endcomponent %}
{% endcomponent %}

{% component "ui_table" rows=rows color="blue" %}
  {% component "ui_table_row" %}
    {% component "ui_table_cell" %}
		{% fill "cell" data="cell_data" %}
          "{{ cell_data|safe }}"
		{% endfill %}
    {% endcomponent %}
  {% endcomponent %}
{% endcomponent %}

Moreover, I won't go too much into it as I've already been writing this for too long, but with provide/inject, one could also define "sections" of the app with particular styling. See v-theme-provider in https://vuetifyjs.com/en/features/theme.

@EmilStenstrom
Copy link
Owner

All this work and new concepts just because "isolated components are easier to maintain" :). I'm starting to doubt that all this work might be more work than having components break sometimes because the all use global scope...

Easier to maintain for the users, of course. Not necessarily the maintainers :)

I'm talking about maintainers :)

@EmilStenstrom
Copy link
Owner

Great examples, thanks for sharing, and putting all that time into constructing them. I definitely see the need for this now.

Now, for the syntax. I wonder if we could use kwargs again?

@JuroOravec
Copy link
Collaborator Author

@EmilStenstrom What do you have in mind with kwargs? Do you mean kwargs in templates, or inside Component class? Also it's important to distinguish between "provide" and "inject".

API for "provide"

So my initial approach was based on Vue. But for example in React it's more common to declare the "provided" state inside the template, e.g.:

<MyCustomProvider myValue=myValue>
  <!-- All components inside "MyCustomProvider" can inject "myValue" -->
</MyCustomProvider>

So in Django that could look like this, if we defined a new tag provide:

{% provide my_value=my_value another_val=one %}
  {# All components inside "provide" can inject "my_value" or "another_val" #}
{% endprovide %}

In the Django example above, ALL kwargs that would be pased to provide tag would be made available for all child components.

Vue- vs React-based approach

The options being:

  • Vue-inspired (define "provide" inside Component class)
  • React-inspired (define "provide" as template tag).

I don't have a preference in using one approach over the other. From my perspective, they are equal, as with both approaches you can achieve the same capabilities.

E.g. what I initially preferred in Vue-inspired approach is that it makes it easy to transform data before the data is provided:

class ParentComp(component.Component):
	provide = ["abc"]
	def get_context_data(self):
		abc = some_crazy_transformation("abc")
		return {
			"abc": abc,
		}
	template = """
		{% component "child" %}
		{% endcomponent %}
	"""

But with React-style provide template tag, the same can be achieved by inserting the provide tag into component template:

class ParentComp(component.Component):
	def get_context_data(self):
		abc = some_crazy_transformation("abc")
		return {
			"abc": abc,
		}
	template = """
		{% provide abc=abc %}
			{% component "child" %}
			{% endcomponent %}
		{% endprovide %}
	"""

OR making a "Provider" component:

@component.register("my_provider")
class MyProvider(component.Component):
	def get_context_data(self):
		abc = some_crazy_transformation("abc")
		return {
			"abc": abc,
		}
	template = """
		{% provide abc=abc %}
			{% slot "content" default %}{% endslot %}
		{% endprovide %}
	"""

class ParentComp(component.Component):
	template = """
		{% component "my_provider" %}
			{% component "child" %}
			{% endcomponent %}
		{% endcomponent %}
	"""

API for "inject"

Injecting via attribute, magic type, or method

As for "injecting" provided values, this one doesn't make sense to define in the template, as the point is to avoid having to use template, so data can be passed multiple levels deep.

So I think that leaves us with 3 approaches:

  1. Using class attribute
    This is the same as in my initial comment, e.g.:

    class ChildComp(component.Component):
    	inject = ["abc", "def"]
    	def get_context_data(self, abc: Any, def: Any):
    		return {
    			"abc": abc,
    			"def": def,
    		}

    Pros: Familiar API

    Cons: Repetitive; injected names may conflict with component input; Not co-located - definition of inject attr could be potentially far from the get_context_data definition.

  2. Using magic types

    class ChildComp(component.Component):
    	def get_context_data(self, abc: Inject(Any), def: Inject(Any)):
    		return {
    			"abc": abc,
    			"def": def,
    		}

    Pros: Not repetitive, Co-located

    Cons: Unfamiliar API - Unfamiliar to users that don't use types; injected names may conflict with component input

  3. Using a method (based on React's useContext)

    class ChildComp(component.Component):
    	def get_context_data(self):
    		abc: Any = self.inject("abc")
    		return {
    			"xyz": 2,
    		}

    Pros: Co-located, Familiar API, doesn't conflict with component input

    Cons: Repetitive

    NOTE: inject MUST be a method (not a standalone function) AND strictly synchronous. Because to implement inject, we'd need to make the current Context temporarily available through the component instance, e.g. via component._context, so component.inject can access it during the execution of get_context_data.

At this point, I'm in favour of using the self.inject method to access the injected data.

Providing/injecting per provider vs per key

Initially I liked the approach I outlined in the first comment, as with these, we don't care about WHO provided the value, but we care only about whether the key is there or not:

{% provide my_value=my_value another_val=one %}
	{% component "child" %}
	{% endcomponent %}
{% endprovide %}
class ChildComp(component.Component):
	inject = ["my_value"]
	def get_context_data(self, my_value):
		print("injected my_value: ", my_value)  # <-- Injected var in get_context_data
		return {
			"xyz": 2,
		}

Alternatively, we could take inspo from React, where you don't say what data you want, but WHO provided it. In that case we would also need to give a "name" property for the provide template tag:

{% provide "my_context" my_value=my_value another_val=one %}
	{% component "child" %}
	{% endcomponent %}
{% endprovide %}

And the injected value would then be a dictionary with all kwargs that were passed to the provide tag:

class ChildComp(component.Component):
	inject = ["my_context"]
	def get_context_data(self, my_context):
		print("injected my_context: ", my_context)  # <-- Injected var in get_context_data
		# {"my_value": ..., "another_val": ...}
		return {
			"xyz": 2,
		}

Don't have a strong preference, here. First I liked the idea of providing data "per key", but it might become repetitive and look ugly with a lot of keys:

class ChildComp(component.Component):
	inject = ["abc", "some_thing", "another_thing", "oh_and_this", "some_more_ids", "another_id"]
	def get_context_data(self, abc, some_thing, another_thing, oh_and_this, some_more_ids, another_id):
		return {
			"xyz": 2,
		}

On the other hand, with the "per provider" approach, we could keep things simpler (assuming that all the keys above are provided by only two providers:

class ChildComp(component.Component):
	inject = ["first_provider", "second_provider"]
	def get_context_data(self, first_provider, second_provider):
		return {
			"xyz": 2,
		}

but it might also feel ugly if we then had to use dict keys to access the data from providers:

class ChildComp(component.Component):
	inject = ["first_provider", "second_provider"]
	def get_context_data(self, first_provider, second_provider):
		first_provider["abc"]
		first_provider["some_thing"]
		first_provider["another_thing"]
		...
		return {
			"xyz": 2,
		}

So I'm considering whether to make it possible to access the keys as attributes:

class ChildComp(component.Component):
	inject = ["first_provider", "second_provider"]
	def get_context_data(self, first_provider, second_provider):
		first_provider.abc
		first_provider.some_thing
		first_provider.another_thing
		...
		return {
			"xyz": 2,
		}

in which case the first_provider would be NamedTuple instead of a dict.

Actually, using a named tuple like that would be nice as it would have some level of immutability baked in (top-level keys immutable).

As a reminder, the keys of the provided values MUST be valid identifiers.

Conclusion

To sum up, personally I'd go with:

  1. provide tag
  2. provide tag would be named, e.g. {% provide "xyz" %}, and when injecting, we would refer the providers' names instead of keys of kwargs passed to providers.
  3. Inside a component, we'd access the provided data with self.inject("provider_name")
  4. The value returned from self.inject would be a NamedTuple

So overall it would look like so:

Provide:

{% provide "my_context" my_value=my_value another_val=one %}
	{% component "child" %}
	{% endcomponent %}
{% endprovide %}

Inject:

class ChildComp(component.Component):
	def get_context_data(self):
		my_context = self.inject("my_context")
		return {
			"abc": my_context.my_value,
			"def": my_context.another_val,
		}

@EmilStenstrom
Copy link
Owner

EmilStenstrom commented May 28, 2024

That was quite the read, so many options to consider!

  • Consideration: If I write a component library, will it work in both "django" and "isolated" mode? I guess all components should be written for isolated, because while isolated might work in django mode, the opposite is not true?
  • I like the provide tag. But I wonder if we should reuse the with tag instead, and make all with variables available everywhere, just like in normal django?

Another option for inject is a decorator. You could specify which global variables to make available, and then access them as if they were passed as args to the get_context_data function. And since we don't use provide, maybe use @global to make it clear that we're accessing the global scope.

class ChildComp(component.Component):
        @global("abc")
	def get_context_data(self, abc):
		print("injected abc: ", abc)  # <-- Injected var in get_context_data
		return {
			"xyz": 2,
		}
	template = """
		abc: {{ abc }} <-- Injected var in template
		xyz: {{ xyz }}
	"""

Thoughts on the {% with %}/@global combo?

@JuroOravec
Copy link
Collaborator Author

JuroOravec commented May 28, 2024

Lol, there's someone called @global, who was tagged 😆 (sorry)

Also, thanks for the feedback & ideas!

Consideration: If I write a component library, will it work in both "django" and "isolated" mode? I guess all components should be written for isolated, because while isolated might work in django mode, the opposite is not true?

Not sure. My take is that it would depend on how it would be implemented. But in theory, components in "django" mode should have access to the same (and more) variables, so it could work, yes.

with tag vs provide tag

I like the provide tag. But I wonder if we should reuse the with tag instead, and make all with variables available everywhere, just like in normal django?

As I'm currently thinking about it, I'd prefer to have the provide tag as a separate tag from with, as they IMO fullfil slightly different needs:

  • with set a variable to be "publicly" available - AKA accessible in the child template freely
  • provide would set a variable to be "privately" available - AKA only to those components that know about them.

From the position of component library author, with tag gives me less confidence that the "provided" data will get to my nested components without the data being modified. Because it's public, and anyone could override it. On the other hand, if the data is passed "privately", then I know that if someone overrode my data, they explicitly called self.inject("my_provider") (or similar). And in such case, it's clearly user's action that messed things up.

E.g. if we had

{% with my_value="abc" %}
	{{ my_value }}

	{% define "def" as my_value %}
	{{ my_value }}

	{% component "child" %}
	{% endcomponent %}
{% endprovide %}
class ChildComp(component.Component):
	@global("my_value")
	def get_context_data(self, my_value):
		print("injected my_value: ", my_value)  # <-- Injected var in get_context_data

Then inside ChildComp.get_context_data, would my_value equal "abc" (AKA value that was registered with with), or "def" (AKA latest value of the variable in the outer scope)?

My take would be that it should be "abc". But if so, then we need to distinguish between the "public" values and "private"/"provided" values, and mark the values set in with tag as "provided".

But if we distinguish between "public" and "provided" values, then it get's more problematic - let's say I have something like below. How will know that first component does NOT have access to my_value, while second does?

{% component "child" %}
{% endcomponent %}

{% with my_value="abc" %}
	{% component "child" %}
	{% endcomponent %}
{% endprovide %}

The problem here is that we don't own the implementation of the with tag. So I think we'd have 2 ways to implement this:

  1. Look at all the Nodes when the Template is parsed, and find all with tags. And then search inside with tags for all component Nodes. And then at render time, inside these components, we would mark the variables from with as "provided". While this sounds feasible, I'm concerned that it would be of similar difficulty as slot tag in terms of maintenance - pieces of logic intertwined throughout the codebase.

  2. Alternatively, we could search for all with Nodes when the Template is parsed, and override the render method on the with Nodes, so we can mark the values as "provided" at render time inside the With tag. However, while I'm kinda OK with monkey-patching other libraries to fix minor bugs like in fix: compat with block tag in django mode #511, I don't like the idea that a major feature would be built on the assumption that the other library doesn't change it's implementaition.

On the other hand, if we had provide tag, all "providing" logic would be neatly encapsulated inside that tag.

Lastly I prefer provide tag for minor things, like what I mentioned in previous comment, that we could have cleaner API, as variables could be injected "per provider" instead of "per variable". And with provide tag, users could also make use of the var:key=value syntax, that's available on component and slot tags.

decorator for injecting data

Another option for inject is a decorator. You could specify which global variables to make available, and then access them as if they were passed as args to the get_context_data function. And since we don't use provide, maybe use @global to make it clear that we're accessing the global scope.

IMO decorator is practically the same what I did as a property on the Component class, so don't mind which of the two if it was between them.

However, I think using a method (e.g. self.inject or similar) is still a bit superior, because it won't cause conflicts if a "provided" value had a same name as component argument.

Because when we consider implementation, then if user does:

class ChildComp(component.Component):
	@global("abc")
	def get_context_data(self, abc):
		print("injected abc: ", abc)  # <-- Injected var in get_context_data

then since abc is part of the function signature for get_context_data, then what happens if I try to supply the same value through the component tag? E.g.

{% component "child" abc="123" %}
{% endcomponent %}

My approach would be that if we define an injected key like @global("abc"), then we should raise an error if user supplied abc as kwarg.

So we'd need to use an exception to handle that situation, whereas we could avoid any exceptions and avoid potential ambiguity we if used the self.inject method.

@EmilStenstrom
Copy link
Owner

@JuroOravec I'm convinced, let's do {% provide %}/self.inject() like you suggest.

@JuroOravec JuroOravec marked this pull request as ready for review June 1, 2024 06:41
@JuroOravec
Copy link
Collaborator Author

@EmilStenstrom Thanks for all your help! I've updated the implementation, added tests, and added a section to README, and it's now ready for review :)

Copy link
Owner

@EmilStenstrom EmilStenstrom left a comment

Choose a reason for hiding this comment

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

Very nicely done as usual. Found some small fixes, but nothing that blocks a release!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/django_components/context.py Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Inlining templates like these in the tests would make the tests much easier to follow.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if we can inline this one (or rather, don't know how), as this file is intended to be used with the {% include %} tag.

@JuroOravec
Copy link
Collaborator Author

@EmilStenstrom Thanks again for all your input and help! I'm excited about this one!

I've updated the docs/comments and added release notes.

@JuroOravec JuroOravec changed the title feat: initial version of inject/provide feat: dependency injection with inject/provide Jun 1, 2024
@JuroOravec JuroOravec merged commit 8ca2814 into EmilStenstrom:master Jun 1, 2024
6 checks passed
@JuroOravec JuroOravec deleted the feat-provide-inject branch June 1, 2024 08:51
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.

3 participants