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

[WebProfilerBundle] Live duration of AJAX request #26668

Merged
merged 1 commit into from Mar 27, 2018

Conversation

Projects
None yet
7 participants
@ostrolucky
Contributor

ostrolucky commented Mar 25, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26645
License MIT
Doc PR -

This will spit out current calculated AJAX request duration, before it finishes. Solves my DX issue about not knowing if request is still in progress.

peek 2018-03-25 15-07

@iltar

This comment has been minimized.

Contributor

iltar commented Mar 26, 2018

This looks pretty useful!

@javiereguiluz

I like this! Before merging, let's wait a bit for reviews from JavaScript experts ... although this already looks as correct as it can be.

@@ -158,6 +158,7 @@
durationCell.className = 'sf-ajax-request-duration';
durationCell.textContent = 'n/a';
row.appendChild(durationCell);
request.updateDuration = setInterval(request.updateDuration, 100, durationCell);

This comment has been minimized.

@stof

stof Mar 26, 2018

Member

using request.updateDuration to store both the callback and the timer identifier after that looks wrong to me. It will make it much easier to introduce bugs by mistake in the future.

@@ -264,6 +267,9 @@
url: url,
method: method,
type: 'fetch',
updateDuration: function(element) {

This comment has been minimized.

@stof

stof Mar 26, 2018

Member

you need to do the same in the code handling XMLHttpRequest. This code does it only for fetch (which will break stuff as startAjaxRequest is used by both)

@@ -264,6 +267,9 @@
url: url,
method: method,
type: 'fetch',
updateDuration: function(element) {
element.textContent = (new Date() - stackElement.start) + 'ms';

This comment has been minimized.

@stof

stof Mar 26, 2018

Member

instead of using a closure to keep a reference to stackElement.start here (which forces you to create the callback in 2 places to make it available for startAjaxRequest), you could instead also pass the start time as an argument in the setInterval call, as you do for the DOM node (and then, you can define the function only once as a sibling of startAjaxRequest and finishAjaxRequest, and reuse it for all requests).

This would also solve the re-purposing of the field.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Mar 26, 2018

Ok how about this? Even less lines and no need to pass arguments to closure \o/

@@ -159,6 +159,10 @@
durationCell.textContent = 'n/a';
row.appendChild(durationCell);
request.liveDurationHandle = setInterval(function() {
durationCell.textContent = (new Date() - request.start) + 'ms';
}, 100);

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

Looks like there is an indentation issue here (tabs?).

This comment has been minimized.

@ostrolucky

ostrolucky Mar 27, 2018

Contributor

indeed

@fabpot

fabpot approved these changes Mar 27, 2018

@stof

stof approved these changes Mar 27, 2018

@stof

This comment has been minimized.

Member

stof commented Mar 27, 2018

@ostrolucky can you rebase your branch to fix conflicts ?

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Mar 27, 2018

sure, done!

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

Thank you @ostrolucky.

@fabpot fabpot merged commit eb6974d into symfony:master Mar 27, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26668 [WebProfilerBundle] Live duration of AJAX request (ost…
…rolucky)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[WebProfilerBundle] Live duration of AJAX request

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26645
| License       | MIT
| Doc PR        | -

This will spit out current calculated AJAX request duration, before it finishes. Solves my DX issue about not knowing if request is still in progress.

![peek 2018-03-25 15-07](https://user-images.githubusercontent.com/496233/37875667-f45376a8-3042-11e8-87e5-24416b8ba670.gif)

Commits
-------

eb6974d [WebProfilerBundle] Live duration of AJAX request

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment