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

Pass endpoint dependencies to Jinja context #4

Closed
hasansezertasan opened this issue Jan 24, 2024 · 7 comments
Closed

Pass endpoint dependencies to Jinja context #4

hasansezertasan opened this issue Jan 24, 2024 · 7 comments

Comments

@hasansezertasan
Copy link
Contributor

We can not access endpoint dependencies from Jinja templates as of now. What if we change this block:

fasthx/fasthx/main.py

Lines 141 to 142 in 2003d8c

def render(result: Any, *, context: dict[str, Any], request: Request) -> HTMLResponse:
return self.templates.TemplateResponse(name=template_name, request=request, context=result)

as this:

        def render(result: Any, *, context: dict[str, Any], request: Request) -> HTMLResponse:
            result.update({"depends": context})
            return self.templates.TemplateResponse(name=template_name, request=request, context=result)

Maybe another name for the key, private instead of depends?

@volfpeter
Copy link
Owner

I'm not sure it can be guaranteed that the mapping returned by the route will always be mutable.

If you need the route's dependencies in addition to its return value for rendering, and you know that the return value is mutable, then I think the best solution (at least for now) is to subclass Jinja and override its __call__() method to do what you described.

@hasansezertasan
Copy link
Contributor Author

hasansezertasan commented Jan 24, 2024

I'm not sure it can be guaranteed that the mapping returned by the route will always be mutable.

That's a problem 😞

If you need the route's dependencies in addition to its return value for rendering, and you know that the return value is mutable, then I think the best solution (at least for now) is to subclass Jinja and override its __call__() method to do what you described.

Well, I'm not actively developing applications with HTMX or Jinja2 for now. So it's not a real deal for me but it probably (most likely) will one day and that day I can subclass this one but it'll be probably a common issue. I think we should think about a solution.

What should we do? Try to find a solution, close this, or keep it open to prevent anyone else from opening a similar issue.

@hasansezertasan
Copy link
Contributor Author

hasansezertasan commented Jan 24, 2024

One idea...

        def render(result: Any, *, context: dict[str, Any], request: Request) -> HTMLResponse:
            request.state.depends = context
            return self.templates.TemplateResponse(name=template_name, request=request, context=result)

This way, we can access the dependencies from request.state.depends and we can set it to a variable inside jinja template to access it easily.

Info: request.state is commonly used for similar purposes, carrying db sessions, user instances, etc.

@volfpeter
Copy link
Owner

I think the best solution is to add another context_factory property to Jinja, which must implement a protocol like this:

class JinjaContextFactory(Protocol):
    def __call__(self, result: Any, route_context: dict[str, Any]) -> dict[str, Any]:
        ...

Additionally, the Jinja class should add at least one default implementation as a classmethod, that simply returns result, and this should be used by default. Maybe result's type hint should be updated, but unfortunately the latest mypy has trouble resolve generics so deep.

Jinja could also have one more default implementation, that returns {**result, "dependencies": route_context}.

This way users could configure their rendering context creation however they need or want to.

@hasansezertasan
Copy link
Contributor Author

Jinja could also have one more default implementation, that returns {**result, "dependencies": route_context}.

This looks good.

@volfpeter
Copy link
Owner

It should be fixed by #11

@hasansezertasan
Copy link
Contributor Author

It should be fixed by #11

Well I didn't checked that but closing this, great work 🚀.

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

No branches or pull requests

2 participants