Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af9225d
added support for window.fetch calls in ajax section
ivoba Aug 9, 2016
e080ca1
applied suggested fixes:
ivoba Aug 24, 2016
36cad1e
added type to profiler table, use 2nd argument instead of catch, usin…
ivoba Oct 5, 2016
385279c
fixed polyfill detect
ivoba Oct 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
applied suggested fixes: 
added error handling on promise catch,
moved fetch block out of XHR block,
fixed method detect,
matched url against excluded urls
  • Loading branch information
@ivoba
ivoba committed Sep 29, 2016
commit e080ca16dbe8b7309b5069d9a4a11a5461114760
31 changes: 22 additions & 9 deletions 31 src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,25 @@
}

{% if excluded_ajax_paths is defined %}
if (window.XMLHttpRequest && XMLHttpRequest.prototype.addEventListener) {
if (window.fetch) {
var oldFetch = window.fetch;
window.fetch = function () {
var promise = oldFetch.apply(null, arguments);

Copy link
Member

@fabpot fabpot Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

extra blank line here

if (window.fetch) {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

var oldFetch = window.fetch;
window.fetch = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutly, fixed

var promise = oldFetch.apply(null, arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

oldFetch.apply(window, arguments);

Copy link
Member

@stof stof Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed using @stof 's approach

Copy link
Contributor

@romainneutron romainneutron Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Works perfectly, no security error, even in cors mode

if (!arguments[0].match(new RegExp({{ excluded_ajax_paths|json_encode|raw }}))) {
var method = 'GET';
if (arguments[1] && arguments[1].method !== undefined) {
method = arguments[1].method;
}

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

Choose a reason for hiding this comment

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

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

method: arguments[1].method,
method: method,
start: new Date()
};

requestStack.push(stackElement);
promise.then(function (r) {
Copy link
Member

Choose a reason for hiding this comment

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

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

stackElement.duration = new Date() - stackElement.start;
Expand All @@ -256,11 +263,17 @@
stackElement.profile = r.headers.get('x-debug-token');
stackElement.profilerUrl = r.headers.get('x-debug-token-link');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@romainneutron any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm gonna try this PR

Sfjs.renderAjaxRequests();
}).catch(function(err) {
Copy link
Contributor

@romainneutron romainneutron Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

stackElement.loading = false;
stackElement.error = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added type to stackElement and to display

});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Sfjs.renderAjaxRequests();
return promise;
};
}
}

return promise;
};
}
if (window.XMLHttpRequest && XMLHttpRequest.prototype.addEventListener) {
var proxied = XMLHttpRequest.prototype.open;

XMLHttpRequest.prototype.open = function(method, url, async, user, pass) {
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.