Closed elineda closed 5 months ago
@matthiask can you review the documentation suggestions?
@matthiask I'm realizing I broke from our typical merge process. @elineda asked in private if she could merge, I said yes.
@tim-schilling No worries. I was a little bit surprised but also relieved. I think merging faster is fine, really, especially in Jazzband.
This is a great change, thanks @elineda! I did notice that this seems to duplicate the functionality of the OBSERVE_REQUEST_CALLBACK
setting, though in a much more discoverable and usable way. I did some looking at the code, and now that the UPDATE_ON_FETCH
setting exists, I cannot come up with a reason for OBSERVE_REQUEST_CALLBACK
to exist. If an AJAX request should not be instrumented, SHOW_REQUEST_CALLBACK
can be used to block that rather than OBSERVE_REQUEST_CALLBACK
(which doesn't actually block the instrumentation, but instead simply prevents an AJAX request from showing up in the history panel until the next time the history panel is loaded or refreshed). Should OBSERVE_REQUEST_CALLBACK
be deprecated in favor of UPDATE_ON_FETCH
?
I went back and read the discussion in https://github.com/jazzband/django-debug-toolbar/pull/1577
I think I agree with your assessment. I remember that I had some problems understanding what OBSERVE_REQUEST_CALLBACK
actually meant, and having only SHOW_REQUEST_CALLBACK
and UPDATE_ON_FETCH
at some point in the future clarifies things a lot for me.
OK, so I just played around with this feature some more, and either I'm doing something wrong, or it is not working the way I expect. I confirmed that I have DjDT 4.3.0 using the Versions panel, and I have an explicit UPDATE_ON_FETCH: False,
in my DEBUG_TOOLBAR_CONFIG
(which should be the same as the default). However, with the Debug Toolbar sidebar expanded, if I trigger an AJAX request, the sidebar still updates to show the data of that AJAX request. The behavior that I want, and was hoping for from UPDATE_ON_FETCH
is that the Debug Toolbar would show the data for the request of the top-level page until I specifically select a different request from the History panel.
I am currently able to achieve that desired behavior by including 'OBSERVE_REQUEST_CALLBACK': lambda request: False,
in my DEBUG_TOOLBAR_CONFIG
. Is there something important I am missing?
OBSERVE_REQUEST_CALLBACK
is meant to identify which AJAX requests need to be instrumented. That setting may not be needed, but it is different from UPDATE_ON_FETCH
. UPDATE_ON_FETCH
is intended to determine whether or not the toolbar will automatically update with the latest AJAX request or not. As you said @living180, it should work the way you expect it to. I'll take a closer look.
@living180 I can't reproduce the issue you're describing. When I run make example
, then go to http://127.0.0.1:8000/ and click on either of the increment buttons, the toolbar doesn't update with the latest request. When I set UPDATE_ON_FETCH
to False, it still doesn't update. When I set UPDATE_ON_FETCH
to True, it does update. When I browse to http://127.0.0.1:8000/htmx/boost/ it works as expected as well. Would you be able to produce a reproducible example for me?
Sure, it's late where I am, but I'll try to reproduce with a simpler example hopefully tomorrow.
Would you be able to produce a reproducible example for me?
OK, turns out the problem on my end was caused by stale cached JavaScript files for the Debug Toolbar. After a force refresh in my browser, UPDATE_ON_FETCH
does behave as expected. Apologies for the noise.
OBSERVE_REQUEST_CALLBACK is meant to identify which AJAX requests need to be instrumented.
I think SHOW_REQUEST_CALLBACK
should be sufficient to identify which requests need to be instrumented, AJAX or not. Both the documentation and the code seem to indicate that OBSERVE_REQUEST_CALLBACK
is simply about whether to automatically update the toolbar in response to an AJAX request or not. Even if OBSERVE_REQUEST_CALLBACK
returns False
, the request will still be instrumented. The only impact on behavior is whether the djdt-store-id
header is included in the response: https://github.com/jazzband/django-debug-toolbar/blob/c688ce4ad7d18c5ecb800869298ca8cf6c08be1d/debug_toolbar/panels/history/panel.py#L27-L28
OBSERVE_REQUEST_CALLBACK
is used to determine if a request is being made on behalf of the toolbar or not. If we were to combine that into SHOW_REQUEST_CALLBACK
, then anytime someone overrides that with return True
, they'll break their integration as the toolbar will attempt to instrument it's own ajax calls. Right now my opinion is that we can't go backwards with that one. It'd break too many existing set ups and expectations.
Unless I'm really reading something wrong, DebugToolbarMiddleware
never instruments its own requests:
In that case the OBSERVE_REQUEST_CALLBACK
callback never gets called, because it only gets called via the HistoryPanel.get_headers()
method:
which is only called by DebugToolbarMiddleware
after the early return show above:
So no matter how SHOW_TOOLBAR_CALLBACK
or OBSERVE_REQUEST_CALLBACK
are defined, the toolbar will never instrument its own requests, AJAX or otherwise.
This also means that the default definition of OBSERVE_REQUEST_CALLBACK
should simply be return True
because it will never be called if DebugToolbar.is_toolbar_request(request)
returns False
.
Ack, good point. I wasn't digging deep enough. I agree that with the new setting, we could direct people to using SHOW_TOOLBAR_CALLBACK
instead of using OBSERVE_REQUEST_CALLBACK
.
I appreciate this enhancement, but very curious why the default is set to False when the historical behavior has been to update with an ajax request? If someone (me) misses this in the changelog, it can make for an annoying hour of troubleshooting. Forget beginner friendly, this is not a developer friendly attitude at all.
There wasn't a great choice for us here. Your experience is the worse case for changing the default. However, that's less worse than the original bug request where the default cause hijacks the toolbar without consent.
I'm sorry you had to deal with the difficult consequences of that choice.
The release notes could have better called it out though.
If I'm remember well, this pr was the reason to not put a minor version on this release.
I appreciate this enhancement, but very curious why the default is set to False when the historical behavior has been to update with an ajax request? If someone (me) misses this in the changelog, it can make for an annoying hour of troubleshooting. Forget beginner friendly, this is not a developer friendly attitude at all.
Thanks for your feedback. You're very welcome to submit a concrete proposal how this could have been handled better and I'd certainly appreciate that. That being said, in my personal opinion the previous behavior wasn't friendly to the development experience of myself and others. Also, all defaults are bad for some subset of users.
Description
[+] Create a setting which enable or disable the automatic refresh when an ajax request occurs.
Fixes #1843
Checklist:
docs/changes.rst
.