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

Update README.rst to clarify this won't work until the response contains a valid html document #416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NewUserHa
Copy link

@NewUserHa NewUserHa commented Feb 12, 2022

What do these changes do?

clarify when this middleware work.
related: https://docs.pylonsproject.org/projects/pyramid-debugtoolbar/en/latest/#usage

Are there changes in behavior for the user?

No

Related issue number

N/A

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@Dreamsorcerer
Copy link
Member

I think the only difference is that the button won't be injected if the response is not HTML. The rest of the features should continue to work, like catching redirects, showing exceptions etc. I think it should be fairly obvious to a developer that the button will not be inserted if there isn't a web page to insert it into, so I'd prefer to avoid using documentation space explaining that.

@NewUserHa
Copy link
Author

NewUserHa commented Feb 13, 2022

for aiohttp.web, the common case is that the users write APIs. And for small tests, the users may try to return a simple response with text='' and content_type='text/html', which is readable to most of the browsers but not aiohttp-debugtoolbar.

So the readme.rst could probably note the users what the middleware requires as the pyramid do.

the readme.rst should be to display a icon instead of current to work probably.

@Dreamsorcerer
Copy link
Member

Maybe we should actually still inject it even if it is missing </head|body> tags?

@NewUserHa
Copy link
Author

agree. But why does pyramid require the closing tags?

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Feb 13, 2022

Probably because it's the sensible place to put it. Without those markers, we will end up creating invalid HTML. But, browsers will generally still parse the invalid HTML just fine (e.g. aiohttp-devtools currently injects code into an invalid location, but still works).

I'd suggest we try to inject into the tag, then check if it failed and append to the end otherwise.

@NewUserHa
Copy link
Author

good idea.
I tested flask_debugtoolbar and pyramid-debugtoolbar, both of them can't display the toolbar without the closing tags.
probably they can improve too.

@NewUserHa
Copy link
Author

BTW, I found the style of the toolbar/fixed panel of django-debug-toolbar and flask_debugtoolbar looks good and is more convenient than this aiohttp one.

@Dreamsorcerer
Copy link
Member

I'm currently refactoring much of the templates/js to make it work correctly with a CSP. But, after that, feel free to propose any changes for styling etc.

@NewUserHa
Copy link
Author

cool!

django-debug-toolbar and flask_debugtoolbar way style is like a single page app.
However, aiohttp-debugtoolbar is a button to go to a new tab.

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