[WebProfiler] added support for window.fetch calls in ajax section #19576

ivoba · 2016-08-09T10:49:54Z

Q	A
Branch?	"master"
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17444
License	MIT
Doc PR	reference to the documentation PR, if any

This adds support for window.fetch calls to the Ajax section of the WebProfiler toolbar.

Credits to @tbopec for implementation :)

fabpot · 2016-08-15T23:03:31Z

@javiereguiluz Can you validate this one?

fabpot · 2016-08-15T23:03:41Z

@robfrawley Can you test it?

nicolas-grekas · 2016-08-23T07:29:43Z

Could anyone please test this one?

stof · 2016-08-23T07:40:42Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                            stackElement.error = r.status < 200 || r.status >= 400;
+                            stackElement.statusCode = r.status;
+                            stackElement.profile = r.headers.get('x-debug-token');
+                            stackElement.profilerUrl = r.headers.get('x-debug-token-link');


Is this compatible with CORS restriction or will it log a security warning ?

@romainneutron any ideas?

I'm gonna try this PR

stof · 2016-08-23T07:44:12Z

Status: needs work

ivoba · 2016-08-24T08:42:00Z

Applied fixes for @stof `s comments.

stof · 2016-08-24T08:42:58Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

@@ -1,4 +1,4 @@
-<script{% if csp_script_nonce is defined and csp_script_nonce %} nonce={{ csp_script_nonce }}{% endif %}>/*<![CDATA[*/


This removal must be reverted. You are breaking the CSP compatibility

stof · 2016-08-24T08:46:21Z

I will review again when the indentation changes are reverted. They make it very hard to see what actually changed (and if I ignored the whitespace changes, I cannot comment on the diff anymore, which is not convenient)

ivoba · 2016-08-24T08:48:32Z

oops how did that happen?
fix is coming...

ivoba · 2016-08-24T08:50:43Z

@stof done, sorryy

aegyed91 · 2016-09-05T13:32:31Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                        var promise = oldFetch.apply(null, arguments);
+
+                        var method = 'GET';
+                        if(arguments[1].method !== undefined){


@ivoba The init config object is optional (Promise<Response> fetch(input[, init]);) therefore this can throw an error. First check if the 2nd argument even exists arguments[1] && arguments[1].method !== undefined .

@tsm91 correct. i fixed this.

fabpot · 2016-09-14T21:35:46Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

@@ -235,6 +235,47 @@
        }

        {% if excluded_ajax_paths is defined %}
+


extra blank line here

fabpot · 2016-09-14T21:35:53Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                var oldFetch = window.fetch;
+                window.fetch = function () {
+                    if (!arguments[0].match(new RegExp({{ excluded_ajax_paths|json_encode|raw }}))) {
+


extra blank line here

fabpot · 2016-09-14T21:36:13Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+
+                        var method = 'GET';
+
+                        if(arguments[1] && arguments[1].method !== undefined){


missing space after if and before {

RolfBabijn · 2016-09-27T13:51:38Z

Really looking forward to this fix/feature. Is there any way I can assist?

stof · 2016-09-27T14:09:38Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                var oldFetch = window.fetch;
+                window.fetch = function () {
+                    if (!arguments[0].match(new RegExp({{ excluded_ajax_paths|json_encode|raw }}))) {
+                        var promise = oldFetch.apply(null, arguments);


this call must be done outside the URL profiling blacklist check (and the return at the end too). Otherwise, you break fetch entirely for URLs blacklisted by the AJAX profiling

@stof fixed

added error handling on promise catch, moved fetch block out of XHR block, fixed method detect, matched url against excluded urls

romainneutron · 2016-09-30T09:16:42Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+            if (window.fetch) {
+                var oldFetch = window.fetch;
+                window.fetch = function () {
+                    var promise = oldFetch.apply(null, arguments);


oldFetch.apply(window, arguments);

even better: .apply(this, arguments), keeping the same context

fixed using @stof 's approach

romainneutron · 2016-09-30T09:32:22Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                            stackElement.profile = r.headers.get('x-debug-token');
+                            stackElement.profilerUrl = r.headers.get('x-debug-token-link');
+                            Sfjs.renderAjaxRequests();
+                        }).catch(function(err) {


it's better to use the second argument of then instead of using catch: if an error is thrown by Sfjs.renderAjaxRequests, the request would be tagged as erroneous whereas the error came from the code executed after that.

romainneutron · 2016-09-30T09:33:05Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                        }).catch(function(err) {
+                            stackElement.loading = false;
+                            stackElement.error = true;
+                        });


what about adding the error content as a property and display this error in the toolbar?

its a good idea, but maybe we can put this in another PR since its also not implemented for XHR, afaik, and it would need some more adjustments in how we render the table.

romainneutron · 2016-09-30T09:35:23Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+            if (window.fetch) {
+                var oldFetch = window.fetch;
+                window.fetch = function () {
+                    var promise = oldFetch.apply(null, arguments);


Works perfectly, no security error, even in cors mode

romainneutron · 2016-09-30T09:36:28Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+                            Sfjs.renderAjaxRequests();
+                        }).catch(function(err) {
+                            stackElement.loading = false;
+                            stackElement.error = true;


what about adding a type so we could make distinction between ajax (xhr) and fetch in the toolbar. As they do not behave the same way, it may be useful, WDYT?

yeah was thinking about that too, but was bit afraid of the extra effort. i see what i can do.

well, the extra work is mainly about figuring out the display. Adding the type property is not hard (it should just be done in both place initializing a stackElement)

i added type to stackElement and to display

romainneutron · 2016-09-30T10:00:40Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

+
+            if (window.fetch) {
+                var oldFetch = window.fetch;
+                window.fetch = function () {


what about using fetch signature (fetch(url, options):promise) instead of relying on arguments? I think it's easier to read

absolutly, fixed

stof · 2016-09-30T10:44:58Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

@@ -235,6 +235,44 @@
        }

        {% if excluded_ajax_paths is defined %}
+
+            if (window.fetch) {


should we try to detect native fetch here ? If someone uses a fetch polyfill based on XHR (e.g. the polyfill maintained by github), we would register the request twice otherwise (at the fetch level and at the xhr level)

how would you detect this? just check if the browser supports fetch? this needs to happen before any ployfill is loaded, im not sure how to do this.
also as the profiler is a dev tool, how many people develop on outdated browsers?

the polyfill defines a window.fetch.polyfill property (set to true) to allow to identify itself.

And people are expected to try their site in old browsers from time to time if they support them. Having your dev tools working to debug errors is a good idea there. The Symfony debug toolbar is meant to work in old browsers, even if it is less fancy (as it is embed in the page itself). The profiler does not support them, but you can always open it in a modern browser in parallel.

ok i see, i added a check on polyfill if (window.fetch && window.fetch.polyfill === undefined)

HeahDude · 2016-10-01T23:02:42Z

@ivoba Do you need help with the templates so you can finish this feature for 3.2? :)

…g fetch signature instead of arguments

fabpot · 2016-10-05T17:36:23Z

Thank you @ivoba.

@tbopec

…lls in ajax section (ivoba) This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19576). Discussion ---------- [WebProfiler] added support for window.fetch calls in ajax section | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#17444 | License | MIT | Doc PR | reference to the documentation PR, if any This adds support for window.fetch calls to the Ajax section of the WebProfiler toolbar. Credits to @tbopec for implementation :) Commits ------- b1b4d70 [WebProfiler] added support for window.fetch calls in ajax section

added support for window.fetch calls in ajax section

af9225d

carsonbot added Status: Needs Review Bug Feature labels Aug 9, 2016

stof reviewed Aug 23, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 23, 2016

stof reviewed Aug 24, 2016
View reviewed changes

ivoba force-pushed the ticket_17444 branch from b8ec1e9 to c4c33f7 Compare August 24, 2016 08:49

aegyed91 reviewed Sep 5, 2016
View reviewed changes

ivoba force-pushed the ticket_17444 branch from c4c33f7 to f6c4e52 Compare September 12, 2016 13:22

fabpot reviewed Sep 14, 2016

View reviewed changes

ivoba force-pushed the ticket_17444 branch from f6c4e52 to 0e72c2e Compare September 15, 2016 12:18

stof requested changes Sep 27, 2016

View reviewed changes

applied suggested fixes:

e080ca1

added error handling on promise catch, moved fetch block out of XHR block, fixed method detect, matched url against excluded urls

ivoba force-pushed the ticket_17444 branch from 0e72c2e to e080ca1 Compare September 29, 2016 08:18

romainneutron suggested changes Sep 30, 2016

View reviewed changes

romainneutron reviewed Sep 30, 2016

View reviewed changes

stof reviewed Sep 30, 2016

View reviewed changes

ivoba added 2 commits October 5, 2016 10:23

added type to profiler table, use 2nd argument instead of catch, usin…

36cad1e

…g fetch signature instead of arguments

fixed polyfill detect

385279c

stof approved these changes Oct 5, 2016

View reviewed changes

fabpot closed this in 2101c10 Oct 5, 2016

patrick-mcdougle mentioned this pull request Oct 10, 2016

Limiting web-profiler-toolbar AJAX request count #20155

Closed

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebProfiler] added support for window.fetch calls in ajax section #19576

[WebProfiler] added support for window.fetch calls in ajax section #19576

		@@ -1,4 +1,4 @@
		<script{% if csp_script_nonce is defined and csp_script_nonce %} nonce={{ csp_script_nonce }}{% endif %}>/<![CDATA[/

		@@ -235,6 +235,47 @@
		}

		{% if excluded_ajax_paths is defined %}


		var method = 'GET';

		if(arguments[1] && arguments[1].method !== undefined){

[WebProfiler] added support for window.fetch calls in ajax section #19576

[WebProfiler] added support for window.fetch calls in ajax section #19576

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment