-
Notifications
You must be signed in to change notification settings - Fork 35
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
Conversation
@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. |
There was a problem hiding this 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.
'clearHistory': clear_history, | ||
} | ||
class InertiaResponse(HttpResponse, BaseInertiaResponseMixin): | ||
json_encoder = settings.INERTIA_JSON_ENCODER |
There was a problem hiding this comment.
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?
response.raise_for_status() | ||
return response.json(), INERTIA_SSR_TEMPLATE | ||
except Exception: | ||
pass |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.")
_page = { | ||
'component': self.component, | ||
'props': self.build_props(), | ||
'url': self.request.get_full_path(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed thanks to @Rey092
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!