Skip to content

[WebProfiler] Improve HttpClient Panel #33315

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2020
Merged

[WebProfiler] Improve HttpClient Panel #33315

merged 1 commit into from
Feb 3, 2020

Conversation

ismail1432
Copy link
Contributor

@ismail1432 ismail1432 commented Aug 23, 2019 •
edited
Loading

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? np
Fixed tickets #33311
License MIT
Doc PR ?

Wishlist from issue :

  • the panel UI could be improved I suppose, e.g. a tooltip on hover in the toolbar
  • putting requests in the "Performance" tab would be great (all timings are already in the collected "info")
  • response bodies are not collected yet [WebProfiler] Improve HttpClient Panel #33315 (comment)
  • when a transport error occurs, (ie the "error" info is populated) we should maybe have a better display too?
  • add a copy-as-curl button next to each request

[EDIT 07 oct 19]
This PR contains :

  • Display a tooltip in Toolbar
  • Add a link only for GET Request in profiler panel
    image

@ismail1432
Copy link
Contributor Author

ismail1432 commented Aug 24, 2019 •
edited
Loading

I started to work on this feature and I have few questions :

putting requests in the "Performance" tab would be great (all timings are already in the collected "info")

Does display the total of request in Performance metrics section is enough (see picture) or we should also implement details for each request in Execution timeline section

image

add a copy-as-curl button next to each request

I just add a href link, I don't know how to implement a copy as curl button in pure html but the user can still copy the link :-)

image

response bodies are not collected yet

Body is returned via toArray IMO it's more readable and it's display a caret thanks to VarDumper ❤️

ro0NL
ro0NL reviewed Aug 24, 2019
View reviewed changes
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/http_client.html.twig Outdated
@@ -8,6 +8,17 @@
<span class="sf-toolbar-value">{{ collector.requestCount }}</span>
{% endset %}

{% set text %}
<div class="sf-toolbar-info-piece">
<b>Total Requests</b>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Total requests

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/http_client.html.twig Outdated
</div>
<div class="sf-toolbar-info-piece">
<b>HTTP errors</b>
<span>{{ collector.errorCount }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class="sf-toolbar-status sf-toolbar-status-red"

@nicolas-grekas
Copy link
Member

Now I'm wondering: where are the request options, which contain the request headers? It might be useful to display them too.

@nicolas-grekas
Copy link
Member

For copy-as-curl, we should be able to borrow from the copy/paste buttons here on the GitHub UI for example.
Then, it's not only about a link, but about all headers. See copy-as-curl from the browser network panel.

@ismail1432
Copy link
Contributor Author

image

Thanks for reaction 👍
Now we display request headers (see picture).
I tried without success to get the Response while downloading 😞
IMO Response body, headers and info should be displayed into a table too instead of being returned as a dump. This is why I removed the dump at this time.

Few questions :
Should we display all request options ? (for me Yes)
We can't implement without pain a copy-as-curl functionality in vanilla JS, can we use third JS library (exemple : https://www.npmjs.com/package/request-to-curl) ?

@nicolas-grekas
Copy link
Member

I personally prefer a compact layout, so that when 10+ requests are made, you don't have to scroll 10+ screens to seem them all. But I'll let others decide what's best as design is not my 1st skill...

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 25, 2019
@stof
Copy link
Member

stof commented Aug 26, 2019

another solution for the case of many requests is to allow collapsing each request (we already have the JS for that)

@nicolas-grekas
Copy link
Member

@ismail1432 up to move this one forward? If there are things that you don't have time to finish, please remove them from the PR so we can merge the ready parts.

@ismail1432
Copy link
Contributor Author

Sorry for the delay, I'll remove asap the part that I can't finish.

@ismail1432
Copy link
Contributor Author

done @nicolas-grekas
I update the description with the content of the PR

@ismail1432 ismail1432 changed the title [WIP][WebProfiler][HttpCLient] Improve HttpClient Panel in WebProfiler [WebProfiler][HttpCLient] Improve HttpClient Panel in WebProfiler Oct 7, 2019
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to master February 3, 2020 17:46
@nicolas-grekas nicolas-grekas changed the title [WebProfiler][HttpCLient] Improve HttpClient Panel in WebProfiler [WebProfiler] Improve HttpClient Panel Feb 3, 2020
@nicolas-grekas
Copy link
Member

Thank you @ismail1432.

nicolas-grekas added a commit that referenced this pull request Feb 3, 2020
@nicolas-grekas
feature #33315 [WebProfiler] Improve HttpClient Panel (ismail1432)
fc30e61 
This PR was merged into the 5.1-dev branch.

Discussion
----------

[WebProfiler] Improve HttpClient Panel

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | np    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #33311  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | ?

Wishlist from issue :

- [x] the panel UI could be improved I suppose, e.g. a tooltip on hover in the toolbar
- [ ] putting requests in the "Performance" tab would be great (all timings are already in the collected "info")
- [ ] ~response bodies are not collected yet~ #33315 (comment)
- [ ] when a transport error occurs, (ie the "error" info is populated) we should maybe have a better display too?
- [x] add a copy-as-curl button next to each request

**[EDIT 07 oct 19]**
This PR contains :
- Display a tooltip in Toolbar
- Add a link only for GET Request in profiler panel
![image](https://user-images.githubusercontent.com/13260307/66316799-389be480-e910-11e9-888e-7703c87c7b10.png)

Commits
-------

e2e6bd0 [WebProfiler] Improve HttpClient Panel
@nicolas-grekas nicolas-grekas merged commit e2e6bd0 into symfony:master Feb 3, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@ro0NL ro0NL ro0NL left review comments

Assignees
No one assigned
Labels
Feature HttpClient Status: Needs Review
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
5 participants
@ismail1432 @nicolas-grekas @stof @ro0NL @carsonbot