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

Deprecated synchronous XHR requests rewritten to async #8205

Closed
wants to merge 3 commits into from
Closed

Deprecated synchronous XHR requests rewritten to async #8205

wants to merge 3 commits into from

Conversation

barbushin
Copy link
Contributor

Fixes #8020

Also I did not refactored sync XHR request in /tests/javascript/index.php:setupContentTrackingFixture() because there was too much problems with qUnit handling async calls in tests.

@barbushin
Copy link
Contributor Author

There was 2 tests failing before this merge http://i.imgur.com/lw8hoYS.png and after all changes JS tests are still failing only on this 2. I think they should be fixed it in some another issue.

@barbushin
Copy link
Contributor Author

There is also relative pull request for CustomAlerts plugin matomo-org/plugin-CustomAlerts#17

@mattab mattab added this to the 2.14.1 milestone Jun 25, 2015
@@ -25,7 +25,7 @@ widgetsHelper.getAvailableWidgets = function () {
widgetsHelper.availableWidgets = data;
}
);
ajaxRequest.send(true);
ajaxRequest.send();
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI: I started refactoring this one a week or two weeks ago and noticed there's definitely some work to do to make it work. Eg this method https://github.com/piwik/piwik/blob/2.14.0-b10/plugins/Dashboard/javascripts/widgetMenu.js#L41 and this method https://github.com/piwik/piwik/blob/2.14.0-b10/plugins/Dashboard/javascripts/widgetMenu.js#L60 accesses it in a sync way but those methods and all callers need to be refactored to work async

Copy link
Member

Choose a reason for hiding this comment

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

If tests work right now, it is more like by accident as the data could be loaded before already but this is not always the case

@tsteur
Copy link
Member

tsteur commented Jun 26, 2015

I'm not sure if this one is for review but already added a few comments. We definitely need to mention the deprecation of sync requests in the changelog (/CHANGELOG.md) and ideally we create a follow up issue to remove it in Piwik 3.0

We will definitely need to carefully test each change manually and check for possible race conditions etc. Eg sometimes we might need to cancel another already running XHR before requesting a new one or we need to make sure that another request is finished before executing another one. Might make sense to use $q library in UI as it won't be realistic to rewrite everything to angular.

Also I did not refactored sync XHR request in /tests/javascript/index.php:setupContentTrackingFixture() because there was too much problems with qUnit handling async calls in tests.

That's absolutely okay

Backward compatibility is kept, adding some @deprecated notices.
@barbushin
Copy link
Contributor Author

@tsteur Please take a loot at https://github.com/barbushin/piwik/commit/1df9fcddda39bb50534ab84635325e6c5c745032 I've fixed widgetsHelper backward compatibility.

@barbushin
Copy link
Contributor Author

I've created new Pull request #8242 with fixes in widgetMenu.js, and without any changes in /tests/javascript/index.php (so JS tests will still use sync calls).

@barbushin barbushin closed this Jun 29, 2015
@tsteur
Copy link
Member

tsteur commented Jun 29, 2015

I'm not sure but it shows quite a lot of "files changed"?

@barbushin
Copy link
Contributor Author

Oh, sorry, I mean #8242

@tsteur
Copy link
Member

tsteur commented Jun 29, 2015

Cheers! Haven't had a look at the other PR's yet. The other one works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants