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

Proposal: Refactor render to create InertiaResponse class #61

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

BrandonShar
Copy link
Collaborator

@BrandonShar BrandonShar commented Jan 6, 2025

Issue #33 mentioned that render being implemented as a method was restricting customization and as I thought about #54 I realized that having a class based response would allow us to better utilize existing Django patterns for creating class based inertia views.

So this refactor would both allow safer customizations in userland AND lay better groundwork for embracing more django features.

Setting this up also required changing how we mock the built in tests, but I think this actually makes them much more straightforward (and work more often! 😄 )

Feedback is super welcome!

@BrandonShar
Copy link
Collaborator Author

@svengt and @mrfolksy I'd be interested in your thoughts here if either of you get some time next week!

@mrfolksy
Copy link
Contributor

mrfolksy commented Jan 7, 2025

@svengt and @mrfolksy I'd be interested in your thoughts here if either of you get some time next week!

@BrandonShar, I'm putting aside some time later this week to do some work on this project, if you are ok to wait for a couple of days.

Copy link
Contributor

@svengt svengt left a comment

Choose a reason for hiding this comment

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

@BrandonShar Very valuable refactor! In my projects I like to use marshmallow to serialize my props. With this change I can easily add this logic by overriding the build_props method in a custom class.

inertia/http.py Outdated Show resolved Hide resolved
inertia/http.py Outdated Show resolved Hide resolved
'clearHistory': clear_history,
}
class InertiaResponse(HttpResponse, BaseInertiaResponseMixin):
json_encoder = settings.INERTIA_JSON_ENCODER
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this attribute should be moved to the mixin, as it depends on it?

inertia/http.py Show resolved Hide resolved
response.raise_for_status()
return response.json(), INERTIA_SSR_TEMPLATE
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to introduce a logger here. This way people can monitor issues with the SSR service.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good idea! Does Django have something standard or would we need something extensible here?

Either way, let's break this idea out into a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to break it into a separate PR.

Python has a built in solution for this. This is an example implementation:

import logging

logger = logging.getLogger(__name__)

...

def build_first_load_context_and_template(self, data):
    if settings.INERTIA_SSR_ENABLED:
      try: 
        ...
      except Exception:
        logger.exception("Error while calling ssr render endpoint.")

inertia/http.py Show resolved Hide resolved
inertia/http.py Outdated Show resolved Hide resolved
_page = {
'component': self.component,
'props': self.build_props(),
'url': self.request.get_full_path(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed thanks to @Rey092

@BrandonShar BrandonShar merged commit 0951ab7 into main Jan 8, 2025
1 check passed
@BrandonShar BrandonShar deleted the refactor-to-class-based-response branch January 8, 2025 15:04
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