(WIP) Adds support to allow Htmx compatibility #1569

gone · 2021-12-29T00:54:04Z

I've been playing with HTMX and djdt and I wanted to get them working better together. HTMX is like turbolinks, in that it loads html from the server and then inserts it into the page without reloading the page as a whole. Unlike turbolinks, it has a lot of support for html fragments, to allow loading chunks of the page in place. There are three modes of use with HTMX - the first page load, an htmx fragment load, and a boosted <a> tag.

Here's what I'm thinking for the three differnt modes of operation:

First page load
Nothing needs to change here. This works exactly like any existing page load.
Fragments
The goal here is to append the toolbar content to the end of the request and allow htmx use the hx-oob (https://htmx.org/attributes/hx-swap-oob/) attribute to just swap out the content. The attribute can be added with the setting value ROOT_TAG_EXTRA_ATTRS

This needs a little more consideration. The issues I've found so far:
- The fragments coming from the server don't have a consistent wrapper. It could be as simple as <p>hey<p> So to update the debug info we can't have a consistent string to look for to decide where to insert the toolbar content.
- The javascript on the page needs to be disabled to prevent multiple click handlers
- The javascript on the page needs to be re-inited after the content is swapped in
- The javascript and css that normally come with the response shouldn't be included

Boosted a tag
This works like turbolinks, and I've been able to get this working pretty well using the same strategy used for turbolinks (Debug toolbar doesn't work with turbolinks after first page load #1179). The fly in the ointment is that unlike the fragment case, this case should NOT include the hx-oob attribute as it's not piggybacking on another request. So I need a way to conditionally include the ROOT_TAG_EXTRA_ATTRS values. Additionally since in this case we've already loaded the css/js we should also exclude those.

An example use of these changes is here: gone/django-hydra@031d4ce#diff-bed13003085067d79e49d240de44a575e7c9c57073f67710eeb0a59fc32bf21aR31

adamchainz · 2022-01-12T13:36:58Z

I don't really like the idea of adding the toolbar HTML to fragment responses. The toolbar HTML will massively outweigh the fragment size.

The "History" panel can already be used to inspect fragments responses, no?

tim-schilling · 2022-01-12T14:07:28Z

The "History" panel can already be used to inspect fragments responses, no?

Yes, it should be captured there.

gone · 2022-01-14T03:54:02Z

I don't really like the idea of adding the toolbar HTML to fragment responses. The toolbar HTML will massively outweigh the fragment size.

I'm open to another approach, but I really like DX of the oob approach. Can you help me understand what cases the html size would be a consideration in?

The "History" panel can already be used to inspect fragments responses, no?

Yes and no - Although it can capture the response, It can't "in the moment". You'll have to refresh the page and then use the history to move back to it, making it much less useful.

I've put up an example of this at https://sampleapp-w4bl.onrender.com/ - it's on the free tier so it's slow.

adamchainz · 2022-01-14T08:39:43Z

Can you help me understand what cases the html size would be a consideration in?

🤷‍♂️ I don't have concrete evidence, but it feels wrong. Certain htmx patterns may fetch many small fragment responses, e.g. for autocompletes. Attaching all the toolbar HTML for every response will massively increase the work done by the browser.

You'll have to refresh the page and then use the history to move back to it, making it much less useful.

The history panel has a "refresh" button which allows you to view new requests without reloading the page. IMO a better approach would be to enhance the history panel functionality, perhaps by making it auto-refresh when opened, or even adding an "auto refresh" mode.

(I tried the refresh button on your demo app and it's broken there, perhaps due to this PR?)

p-baum · 2022-01-14T13:08:11Z

I don't really like the idea of adding the toolbar HTML to fragment responses. The toolbar HTML will massively outweigh the fragment size.

I dont see this as a valid reason. The debug toolbar is only used during development and debugging. Why is size important in this context? OOB makes sense to me.

The "History" panel can already be used to inspect fragments responses, no?

No. Its not working for me with fragments. In fact its throwing errors:

Uncaught (in promise) TypeError: inner.previousElementSibling is null
    init http://localhost:8000/static/debug_toolbar/js/toolbar.js:45
    promise callback*init/< http://localhost:8000/static/debug_toolbar/js/toolbar.js:44
    on http://localhost:8000/static/debug_toolbar/js/utils.js:6
    on http://localhost:8000/static/debug_toolbar/js/utils.js:3
    init http://localhost:8000/static/debug_toolbar/js/toolbar.js:14
    <anonymous> http://localhost:8000/static/debug_toolbar/js/toolbar.js:307
toolbar.js:45:29
    init http://localhost:8000/static/debug_toolbar/js/toolbar.js:45
    (Async: promise callback)
    init http://localhost:8000/static/debug_toolbar/js/toolbar.js:44
    on http://localhost:8000/static/debug_toolbar/js/utils.js:6
    (Async: EventListener.handleEvent)
    on http://localhost:8000/static/debug_toolbar/js/utils.js:3
    init http://localhost:8000/static/debug_toolbar/js/toolbar.js:14
    <anonymous> http://localhost:8000/static/debug_toolbar/js/toolbar.js:307
    InnerModuleEvaluation self-hosted:2388
    evaluation self-hosted:2349

p-baum · 2022-01-14T13:18:16Z

Certain htmx patterns may fetch many small fragment responses, e.g. for autocompletes. Attaching all the toolbar HTML for every response will massively increase the work done by the browser

Simply update the toolbar after every response. Sure with autocomplete this will slow the browser down. But remember. Were debugging here. Why does it need to be fast?

gone · 2022-01-14T14:57:08Z

I think the auto-refresh is a really good idea and it's applicable for a lot of different usecases like vanilla ajax requests. Let me get a PoC up on that to see how it feels

p-baum · 2022-01-14T15:34:22Z

We could always defend against browser overload by debouncing or something IF it becomes a problem.

gone · 2022-01-16T17:07:35Z

Ok Initial PoC for auto refresh mode.

I would love any feedback/suggestions but the core changes to get this working:

The response that is switchable will now contains headers that contain the store id, signature and url to get data from (https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-4c34c65dc0e8564d6d1b67b93a2099a4a8f7a5264cded1f09de5eb724bb6523dR24)
The javascript around the switching function has been broken out so that you can switch through the history panel's button or by passing a store id/data (https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-b62b57713c8d077f5a638ab38f847bbc0b6711016acaf14c1f9ec1f8ff100605R130)
An annotation for the XHR prototype that watches all ajax requests and updates the toolbar when appropriate (https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-ec4138914976be8d39d9780b045095caba36dfae38e8f8e671279b0d41e3aad6R323)

I would love any feedback about the approach!

Some open questions I have from working through this:

Why are we signing the request for the switch request? We don't sign loading the panel so I'm wondering if this can be removed or if it serves a purpose

I am calling toolbar.store twice: https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-4c34c65dc0e8564d6d1b67b93a2099a4a8f7a5264cded1f09de5eb724bb6523dR27 and in the main render call. Can we remove the second one? The comment there says it's being doing for the purpose of the history panel.

This is not in a merge ready state and I have some clean up to do. Some of the changes from the prior approach I need to discard as they no longer matter, and some of them are still important for the boosted case.

Making a todo list for myself:

Fix git rebase snafoo
Come up with Better names for the headers
remove functionality we longer need
Properly wrap the js prototype annotation into the DJDT namespace, and initialize in the template
Property wrap the the prototype initialization around a setting value
Test
documentation
BUG: https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-9544c759ef64d0c6ba09293b8398aa6a00311339b47afec022bc66af161e6312L25 breaks the historical panel behavior. I'm planning on popping the value client side to get the old behavior, but I could also update the view to take an optional form key to make the decision server side. LMK if there's a preference
BUG: When switching between pages (htmx boosted mode) a panel that was already loaded in a prior page doesn't have it's scripts reinitialized when loading in the new page

adding view that will return the toolbar for a given store id Adding header containing toolbar url Refactoring history javascript to allow switching state by store id

tim-schilling

Thanks for the work on this so far. It would be great if you could pull out the auto-refresh code into it's own PR. I feel like the scope of that should be smaller than the changes to support HTMX.

A general concern is that the changes to support auto-refreshing of the toolbar's history panel is that I couldn't find the spot that actually does the auto-refreshing. Can you give me a little bit of a rundown of how your changes are supporting that?

Why are we signing the request for the switch request? We don't sign loading the panel so I'm wondering if this can be removed or if it serves a purpose

Admittedly, it may not be necessary, but the purpose of it is to verify that the client isn't changing the store id of the toolbar that is being requested.

I am calling toolbar.store twice: https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-4c34c65dc0e8564d6d1b67b93a2099a4a8f7a5264cded1f09de5eb724bb6523dR27 and in the main render call. Can we remove the second one? The comment there says it's being doing for the purpose of the history panel.

We can't rely on all of the panels always being enabled. If the only call to toolbar.store() was made in the HistoryPanel, we run the risk of it never being called if the HistoryPanel is not included.

tim-schilling · 2022-01-16T21:03:16Z

debug_toolbar/static/debug_toolbar/js/utils.js

@@ -91,6 +106,14 @@ function ajax(url, init) {
        });
 }

+function pluckData(array, key) {


This function can likely be replaced with:

return array.map(function(obj) { return obj.dataset[key] })

That was my first attempt, but annoyingly Set objects don't have a map function.

tim-schilling · 2022-01-16T21:14:49Z

debug_toolbar/static/debug_toolbar/js/toolbar.js

+    abort() {
+        controller.abort();
+        resetAbortController();
+    },


This looks like the only part of the code that uses the controller and the signal. Outside of allowing another third party panel to abort a request, what purpose does this serve?

Calling abort will unregister the click handlers - the intent here is that init will configure the JS click handlers and this will undo it to put us back in the state we were immediately before init.

The Purpose of that is that for boosted requests / Turbolinks the new page will contain it's own toolbar html, and I was finding conflicts with multiple click handlers registered.

tim-schilling · 2022-01-16T21:27:44Z

debug_toolbar/panels/history/panel.py

+            sig = SignedDataForm(
+                initial=HistoryStoreForm(initial={"store_id": store_id}).initial
+            ).initial.get("signed")
+            response["dj-history-sidebar-url"] = reverse("djdt:history_sidebar")


Since this isn't likely to change between requests, this could be moved out of the per-request cycle and into the template as a JS variable.

tim-schilling · 2022-01-16T21:29:03Z

debug_toolbar/panels/history/panel.py

+        if not self.toolbar.should_render_panels():
+            self.toolbar.store()
+            store_id = self.toolbar.store_id
+            response["DJ-TOOLBAR-BASE-URL"] = (


I don't think this is being used.

It is not, I will remove

gone

if you could pull out the auto-refresh code into it's own PR

I agree - I think the line is going to be fragment responses will probably be handled by the "auto-refresh" stuff and the htmx specific changes will be around boosted links. Let me make a test case with a vanilla hxr/fetch update.

Can you give me a little bit of a rundown of how your changes are supporting that?

That's happening here: https://github.com/jazzband/django-debug-toolbar/pull/1569/files#diff-ec4138914976be8d39d9780b045095caba36dfae38e8f8e671279b0d41e3aad6R323
An ajax request is made by the client, and when it comes back from the server that JS function will inspect the incoming request, and if it's a good candidate for updating will make a follow up js request to grab the toolbar data and switch it in.

gone · 2022-01-16T21:32:39Z

debug_toolbar/panels/history/panel.py

+        if not self.toolbar.should_render_panels():
+            self.toolbar.store()
+            store_id = self.toolbar.store_id
+            response["DJ-TOOLBAR-BASE-URL"] = (


It is not, I will remove

gone · 2022-01-16T21:33:00Z

debug_toolbar/panels/history/panel.py

+            sig = SignedDataForm(
+                initial=HistoryStoreForm(initial={"store_id": store_id}).initial
+            ).initial.get("signed")
+            response["dj-history-sidebar-url"] = reverse("djdt:history_sidebar")


gone · 2022-01-16T21:37:10Z

debug_toolbar/static/debug_toolbar/js/toolbar.js

+    abort() {
+        controller.abort();
+        resetAbortController();
+    },


Calling abort will unregister the click handlers - the intent here is that init will configure the JS click handlers and this will undo it to put us back in the state we were immediately before init.

The Purpose of that is that for boosted requests / Turbolinks the new page will contain it's own toolbar html, and I was finding conflicts with multiple click handlers registered.

gone · 2022-01-16T21:38:10Z

debug_toolbar/static/debug_toolbar/js/utils.js

@@ -91,6 +106,14 @@ function ajax(url, init) {
        });
 }

+function pluckData(array, key) {


That was my first attempt, but annoyingly Set objects don't have a map function.

gone · 2022-01-17T00:20:43Z

Closing this in favor of #1577 which I think is a more fundamental case. I'll reopen this once that's got a clear way forward and this can focus on just htmx stuff (I think just the boosted link case)

gone added 5 commits December 28, 2021 19:29

adding abort signal to turn off the js

2e310bf

moving decision to insert into function to allow overriding

3cb6f71

adding slot to dynamically configure toolbar

97330ee

Optionally render js/css

4068382

Dynamic values for root_tag_extra_attrs

b6a377b

gone changed the title ~~Adds support to allow Htmx compatability~~ Adds support to allow Htmx compatibility Dec 29, 2021

tim-schilling mentioned this pull request Jan 14, 2022

Support auto-refresh of the history panel #1575

Closed

gone force-pushed the htmx branch from fe4553f to 9725f9a Compare January 16, 2022 16:40

gone changed the title ~~Adds support to allow Htmx compatibility~~ (WIP) Adds support to allow Htmx compatibility Jan 16, 2022

gone changed the base branch from main to develop January 16, 2022 16:51

gone changed the base branch from develop to main January 16, 2022 16:52

gone force-pushed the htmx branch from 9725f9a to 355b214 Compare January 16, 2022 17:00

gone added 3 commits January 16, 2022 15:56

allowing history refresh and switch to be scripted

7e61c10

Work in preperation of an autorefresh

a931fed

adding view that will return the toolbar for a given store id Adding header containing toolbar url Refactoring history javascript to allow switching state by store id

Adding initial autorefresh mode

32806b8

gone force-pushed the htmx branch from 355b214 to 32806b8 Compare January 16, 2022 20:56

removing some features we don't need

a47ae06

gone force-pushed the htmx branch from 6d163d2 to a47ae06 Compare January 16, 2022 21:31

tim-schilling reviewed Jan 16, 2022

View reviewed changes

removing render_base since we're using json not html

7468906

gone commented Jan 16, 2022

View reviewed changes

gone added 2 commits January 16, 2022 17:47

Adds a setting value to toggle observing ajax requests

359bb0d

Fix bug with switching inside the history panel

dfc6aa6

gone closed this Jan 17, 2022

(WIP) Adds support to allow Htmx compatibility #1569

(WIP) Adds support to allow Htmx compatibility #1569

Uh oh!

Conversation

gone commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamchainz commented Jan 12, 2022

Uh oh!

tim-schilling commented Jan 12, 2022

Uh oh!

gone commented Jan 14, 2022

Uh oh!

adamchainz commented Jan 14, 2022

Uh oh!

p-baum commented Jan 14, 2022

Uh oh!

p-baum commented Jan 14, 2022

Uh oh!

gone commented Jan 14, 2022

Uh oh!

p-baum commented Jan 14, 2022

Uh oh!

gone commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tim-schilling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gone commented Jan 17, 2022

Uh oh!

Uh oh!

gone commented Dec 29, 2021 •

edited

Loading

gone commented Jan 16, 2022 •

edited

Loading