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

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

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
9 participants
@ivoba
Copy link
Contributor

commented Aug 9, 2016

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

@javiereguiluz Can you validate this one?

@fabpot

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

@robfrawley Can you test it?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Could anyone please test this one?

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');

This comment has been minimized.

Copy link
@stof

stof Aug 23, 2016

Member

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

This comment has been minimized.

Copy link
@fabpot

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

I'm gonna try this PR

@@ -236,6 +236,31 @@
{% if excluded_ajax_paths is defined %}
if (window.XMLHttpRequest && XMLHttpRequest.prototype.addEventListener) {
if (window.fetch) {

This comment has been minimized.

Copy link
@stof

stof Aug 23, 2016

Member

this does not need to be inside the check for window.XMLHttpRequest

var stackElement = {
loading: true,
error: false,
url: arguments[0],

This comment has been minimized.

Copy link
@stof

stof Aug 23, 2016

Member

you need to match the URL against the excluded ajax paths before adding it in the request stack

start: new Date()
};
requestStack.push(stackElement);
promise.then(function (r) {

This comment has been minimized.

Copy link
@stof

stof Aug 23, 2016

Member

what if the promise gets rejected ? The stack element should not stay in loading state in this case

@stof

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Status: needs work

@ivoba

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

Applied fixes for @stof `s comments.

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

This comment has been minimized.

Copy link
@stof

stof Aug 24, 2016

Member

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

collectionToArray = function (collection) {
var length = collection.length || 0,
results = new Array(length);
collectionToArray = function (collection) {

This comment has been minimized.

Copy link
@stof

stof Aug 24, 2016

Member

please revert all these indentation changes

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

This comment has been minimized.

Copy link
@stof

stof Aug 24, 2016

Member

applying the original fetch (and returning the promise) must still be made even when the URL is excluded from the profiling, otherwise you are breaking window.fetch

@stof

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

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

@ivoba ivoba force-pushed the freshcells:ticket_17444 branch from b8ec1e9 to c4c33f7 Aug 24, 2016

@ivoba

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

@stof done, sorryy

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

This comment has been minimized.

Copy link
@tsm91

tsm91 Sep 5, 2016

@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 .

This comment has been minimized.

Copy link
@ivoba

ivoba Sep 12, 2016

Author Contributor

@tsm91 correct. i fixed this.

@ivoba ivoba force-pushed the freshcells:ticket_17444 branch from c4c33f7 to f6c4e52 Sep 12, 2016

@@ -235,6 +235,47 @@
}
{% if excluded_ajax_paths is defined %}

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 14, 2016

Member

extra blank line here

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

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 14, 2016

Member

extra blank line here

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

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 14, 2016

Member

missing space after if and before {

@ivoba ivoba force-pushed the freshcells:ticket_17444 branch from f6c4e52 to 0e72c2e Sep 15, 2016

@Gadgetdude

This comment has been minimized.

Copy link

commented Sep 27, 2016

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

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 comment has been minimized.

Copy link
@stof

stof Sep 27, 2016

Member

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

This comment has been minimized.

Copy link
@ivoba

ivoba Sep 29, 2016

Author Contributor

@stof fixed

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

@ivoba ivoba force-pushed the freshcells:ticket_17444 branch from 0e72c2e to e080ca1 Sep 29, 2016

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

oldFetch.apply(window, arguments);

This comment has been minimized.

Copy link
@stof

stof Sep 30, 2016

Member

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

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

fixed using @stof 's approach

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

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.

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

fixed

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

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

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

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.

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

Works perfectly, no security error, even in cors mode

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

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?

This comment has been minimized.

Copy link
@ivoba

ivoba Sep 30, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@stof

stof Sep 30, 2016

Member

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)

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

i added type to stackElement and to display

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

This comment has been minimized.

Copy link
@romainneutron

romainneutron Sep 30, 2016

Member

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

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

absolutly, fixed

@@ -235,6 +235,44 @@
}
{% if excluded_ajax_paths is defined %}
if (window.fetch) {

This comment has been minimized.

Copy link
@stof

stof Sep 30, 2016

Member

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)

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

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?

This comment has been minimized.

Copy link
@stof

stof Oct 5, 2016

Member

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.

This comment has been minimized.

Copy link
@ivoba

ivoba Oct 5, 2016

Author Contributor

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

@HeahDude

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

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

ivoba added some commits Oct 5, 2016

added type to profiler table, use 2nd argument instead of catch, usin…
…g fetch signature instead of arguments
@stof

stof approved these changes Oct 5, 2016

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

Thank you @ivoba.

@fabpot fabpot closed this in 2101c10 Oct 5, 2016

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

feature symfony#19576 [WebProfiler] added support for window.fetch ca…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.