EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
973 stars 62 forks source link

Dependency rendering overhaul #35

Closed rbeard0330 closed 3 years ago

rbeard0330 commented 3 years ago

As promised in #29. I wasn't able to find a way to make this work well using template tags alone. The problem is that CSS dependencies especially are rendered before most or all of the components, and there's not a great way to know what components will be rendered at that time. Django-sekizai deals with this problem by effectively having the first tag that needs to write dynamic content effectively hijack the parser, and read in the rest of the entire template. This allows it to render everything else first, see what happens, and then write its dynamic content at the right place in the output.

I didn't follow that approach since: 1) if people want to use sekizai to manage JS/CSS dependencies, they already can, and 2) that approach is pretty drastic and might break things in some cases. Instead, the approach here is to use a middleware to post-process the fully rendered response with the necessary dependencies. More specifically:

  1. At the start of the render cycle, the middleware injects an empty set into the context.
  2. When each component node is rendered, it adds itself to the set.
  3. When a dependency tag is rendered, it just renders a static placeholder element.
  4. After the request has been rendered, the middleware comes back in, uses the set of rendered components to determine what dependencies are requested, and drops those dependencies in where the static placeholder element is.

Limitations:

  1. SIGNIFICANT The middleware relies on special processing hooks that are only available for TemplateResponse objects, so this will not work with a normal HttpResponse. All of the Django CBVs return TemplateResponses, but Django's render shortcut returns a regular HttpResponse.
  2. If a component is rendered in an isolated context ( i.e., {% include ___ with x=1 only %}), then it can't register to get its dependencies. I did modify the only behavior of the component and component_block tags to still pass through the dependency tracker.

Still to do:

  1. For now, I just took over the existing component dependency tags to use with the middleware, but that isn't backwards compatible, so that should be fixed. I think a reasonable approach is that if the middleware isn't active (either because not installed or because it's not a TemplateResponse), then we can fall back to the legacy approach of rendering all dependencies.
  2. It would be nice to allow users to add JS/CSS dependencies to the heap even if they aren't associated with a component. This would allow those to be deduplicated and ordered through Django's Media objects. We could get the media from Forms too.
  3. Documentation.
  4. I added a setting to render scripts as modules, which is just a global toggle. We probably want to allow more customization so that users can add nonces and do other stuff. Not yet sure how best to do that.

Look forward to getting your thoughts!

rbeard0330 commented 3 years ago

I think your suggestion would work well, and would remove the TemplateResponse restriction. I think we can use the component name as the key, since that needs to be unique anyways. So the flow would be:

  1. Components detect whether the middleware is active (I think we can just check the settings), and if it is, they add an HTML comment with a django_components identifier plus the component name.
  2. render_dependencies tags also check whether the middleware is active. If not, they just render dependencies for all registered components (as they do currently). If it is, they render a placeholder.
  3. The middleware does nothing on the inbound leg. Then when it gets a response, it checks whether its HTML.
  4. If it is an HTML response, we use regex to extract all the component names from the HTML comments.
  5. Then the middleware collects the media objects corresponding to the component names and sums them up.
  6. Then we do a second regex pass to insert the dependencies and remove the dependency placeholders and the component_name comments.

I think this should work, and it should also work in isolated contexts. I'll get to work on implementing and addressing your comments. I'll also do a before-and-after bench to see if there's an impact on performance. If there is a significant performance hit, we could use the TemplateResponse approach as the default, and then fall back to the text-based solution for HttpResponses. That's more complexity though.

EmilStenstrom commented 3 years ago

@rbeard0330 Thank you for working on this! Great that you'll look at performance too, that was a slight worry for me too.

EmilStenstrom commented 3 years ago

@rbeard0330 Not to rush you, but will you be working on this in the near future? I have some fixes around adding black formatting to the code base, but I'd hate to make your code hard to merge because of it. Let me know if there's something I can do to help!

rbeard0330 commented 3 years ago

Sorry, I ran into some headwinds and then got distracted. I was able to get a new version together though, and just committed it. Let me know what you think once you've had a chance to review.

EmilStenstrom commented 3 years ago

@rbeard0330 Thank you! There are a few unanswered questions above. Would you mind going through them and either answer them or make a commit and resolve them? It's a little tricky to know what change fixed what comment right now.

rbeard0330 commented 3 years ago

Sure. I'll run through those today.

EmilStenstrom commented 3 years ago

Thank you so much! I made a copy of this PR in #52 without the first two commits in your PR. I also fixes some flake8 errors, which strangely does not get picked up my running tests on GitHub Actions.