volfpeter / fasthx

FastAPI server-side rendering with built-in HTMX support.
https://volfpeter.github.io/fasthx/
MIT License
458 stars 9 forks source link

Returning HX-* headers with response #18

Closed cubinet-code closed 8 months ago

cubinet-code commented 8 months ago

Hi,

As HTMX relies on returning HX-* Header for certain functionality like triggers, I was wondering how to send these with fasthx?

I tried like this, but that seems to get filtered for HTMX responses:

response.headers["HX-Trigger"] = "newPortfolio"

# Add new
@router.post("/", response_model=PortfolioCreate)
@jinja.hx("portfolios/portfolio.jinja2")
async def create(response: Response, portfolio: PortfolioCreate, db: Session = Depends(database.get_db)):
    response.headers["HX-Trigger"] = "newPortfolio"
 ...
    return record

The json response is correct though:

< HTTP/1.1 200 OK
< date: Tue, 26 Mar 2024 11:26:16 GMT
< server: uvicorn
< content-length: 38
< content-type: application/json
< hx-trigger: newPortfolio
< 
* Connection #0 to host localhost left intact
{"name":"Portfolio3","description":""}

Ollie.

volfpeter commented 8 months ago

Hi,

Thanks for reporting the issue in such detail!

Yes, the header you set on response is discarded because hx() and page() internally creates an HTMLResponse and it doesn't copy the headers you set inside the route.

Maybe this missing feature could be implemented simply by adding a __response: Response parameter to the wrapper method within hx() and page(). Copying headers and session data could be helpful for other scenarios as well. I'll need to check whether the Response object supports this. If it doesn't work, then maybe a special dependency class could be added to lib: all response attributes could be set on that object, and the lib would take care of applying those changes to the created response. I'll think about this.

If you'd like to give the implementation a go, I'd be happy to get a PR :slightly_smiling_face: Otherwise I'll try to get it done when I have a bit of time.

Peter

volfpeter commented 8 months ago

I couldn't resist. The feature itself is ready. I still need to add tests and docs.

If having this feature is very urgent, I can publish a new release without these.

cubinet-code commented 8 months ago

Haha. Thank you. I was looking at your code just now too :) I can just checkout the branch you committed to for the time being.

best regards,

Oliver.

cubinet-code commented 8 months ago

by the way. Thanks for this module. It's much more elegant than the fastapi-htmx I tried earlier.

volfpeter commented 8 months ago

Thanks :slightly_smiling_face:

volfpeter commented 8 months ago

See the linked PR. I'd appreciate a comment there, just to make sure the feature fixes your issue.

volfpeter commented 8 months ago

Version 0.2403.0 has been deployed to PyPI with the requested feature. As mentioned in the release notes, this is probably the last release before the project goes sem-ver.

cubinet-code commented 8 months ago

Not sure you get notified for comments in a closed PR:

In my case at least rendered is already an instance of Response, hence the headers would have to be merged. This works for me:

            if isinstance(rendered, str):
                return HTMLResponse(
                    rendered, headers=None if response is None else response.headers
                )
            else:
                rendered.headers.update({} if response is None else response.headers)
                return rendered

But maybe I am not seeing the bigger picture. I am not too sure for which cases isinstance(rendered, str) is checked?

Calling code:

# Get all
@router.get("/")
@jinja.hx("portfolios/portfolios.jinja2")
async def get_portfolios(response: Response, db: Session = Depends(database.get_db)) -> list[dict]:
    response.headers["HX-Trigger"] = "newPortfolio"
    records = db.query(Portfolio).all()
    return records
volfpeter commented 8 months ago

Ah, right. I tested with the custom templating version, which worked, and I stupidly assumed the Jinja-version will also work by default, but it doesn't.

volfpeter commented 8 months ago

Okay, I added the missing Response support to Jinja by internally converting the template response back into a string. This way the core hx() and page() decorators do the response header update as expected. There are minor drawbacks for this solution, but nothing notable I think.

If a more complex solution becomes necessary, the right way to implement it would be probably to introduce a custom response class, which would signal to hx that it should execute the same logic on the response as for strings.

volfpeter commented 8 months ago

Version 0.2403.1 is out on PyPI with the fix.