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

Make AJAX views return JSON instead of HTML #1282

Merged
merged 1 commit into from
May 19, 2020
Merged

Make AJAX views return JSON instead of HTML #1282

merged 1 commit into from
May 19, 2020

Conversation

jdufresne
Copy link
Contributor

Returning JSON allows for arbitrary data to be returned, not only HTML.
This in turn allows for returning scripts as a list instead of embedded
in the HTML. The script tag embedded in the HTML didn't immediately
execute anyway, so handling it as separate data makes more sense.

Separating the JavaScript makes it simpler to migrate to JavaScript
modules as inserting the new scripts is handled entirely by toolbar.js.

Removes the unused $$.executeScripts() call in the .remoteCall AJAX
handler. No panels using .remoteCall inject scripts in this way.

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #1282 into master will increase coverage by 0.21%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
+ Coverage   86.53%   86.74%   +0.21%     
==========================================
  Files          25       25              
  Lines        1441     1449       +8     
  Branches      206      205       -1     
==========================================
+ Hits         1247     1257      +10     
  Misses        141      141              
+ Partials       53       51       -2     
Impacted Files Coverage Δ
debug_toolbar/panels/timer.py 79.66% <50.00%> (-3.36%) ⬇️
debug_toolbar/panels/sql/views.py 91.04% <100.00%> (+0.41%) ⬆️
debug_toolbar/panels/templates/views.py 62.79% <100.00%> (+0.88%) ⬆️
debug_toolbar/panels/profiling.py 91.07% <0.00%> (+1.78%) ⬆️
debug_toolbar/middleware.py 95.00% <0.00%> (+4.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 799f585...af0d5af. Read the comment docs.

@matthiask
Copy link
Member

I like this very much. At the same time, there is no backwards compatibility for third party panels or for pending pull requests (e.g. #1250) -- do you think it would be possible to add a fallback? Or is it even worth it? (The backend changes for individual panels are small after all.)

@jdufresne
Copy link
Contributor Author

there is no backwards compatibility for ... for pending pull requests (e.g. #1250)

I took a very quick look at this PR. It looks like it adds a new JavaScript function ajaxJson, so if we move all AJAX requests to JSON, that function will no longer be needed. In short, I think these two PRs are very compatible with one another.

If PR #1250 lands first, I'll happily amend this PR to take it into account. If, on the other hand, this PR lands first, I think PR #1250 could be easily simplified a bit by using ajax. I'm happy to help out there as well.

At the same time, there is no backwards compatibility for third party panels ... -- do you think it would be possible to add a fallback? Or is it even worth it? (The backend changes for individual panels are small after all.)

Is there an existing policy for this? If there is, I'll happily follow it. But, it will complicate the JavaScript code a bit to handle two different server side APIs.

Is it worth it? This is hard to predict and perhaps a personal opinion. On one hand, the compatibility shim adds complication to django-debug-toolbar, but on the other not adding one requires action from third party panel maintainers if they wish to add support for the next djdt version.

IMO, the changes are small enough and the third party panel community is also small enough such that adding the compatibility shim is not worth it. But I'm happy to add it anyway if it helps move the PR forward. So please let me know your preference and I'll follow it.

Something to keep in mind, if a compatibility shim is added, it will eventually be dropped in a future version leading us back to the same question. So we'll expect third party panels to make changes down the road anyway.

Either way, some words should be added to the release notes, I'll go ahead and do that before this should be merged. If we feel the API change is large enough, we can also bump the major release number too.

Returning JSON allows for arbitrary data to be returned, not only HTML.
This in turn allows for returning scripts as a list instead of embedded
in the HTML. The script tag embedded in the HTML didn't immediately
execute anyway, so handling it as separate data makes more sense.

Separating the JavaScript makes it simpler to migrate to JavaScript
modules as inserting the new scripts is handled entirely by toolbar.js.

Removes the unused $$.executeScripts() call in the .remoteCall AJAX
handler. No panels using .remoteCall inject scripts in this way.
@jdufresne
Copy link
Contributor Author

Added docs and made a note in chagnes.rst in the latest revision.

@matthiask
Copy link
Member

Thanks for the docs and for the explanation. I wholeheartedly agree.

@matthiask matthiask merged commit 08bb474 into django-commons:master May 19, 2020
@jdufresne jdufresne deleted the json-ajax branch May 19, 2020 12:57
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.

2 participants