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
Conversation
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. |
There is also relative pull request for CustomAlerts plugin matomo-org/plugin-CustomAlerts#17 |
@@ -25,7 +25,7 @@ widgetsHelper.getAvailableWidgets = function () { | |||
widgetsHelper.availableWidgets = data; | |||
} | |||
); | |||
ajaxRequest.send(true); | |||
ajaxRequest.send(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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
That's absolutely okay |
Backward compatibility is kept, adding some @deprecated notices.
@tsteur Please take a loot at https://github.com/barbushin/piwik/commit/1df9fcddda39bb50534ab84635325e6c5c745032 I've fixed |
I've created new Pull request #8242 with fixes in |
I'm not sure but it shows quite a lot of "files changed"? |
Oh, sorry, I mean #8242 |
Cheers! Haven't had a look at the other PR's yet. The other one works :) |
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.