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

[Vue] add promise api to ajaxHelper and deprecate piwikApi service #18114

Merged
merged 22 commits into from Oct 13, 2021

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Oct 8, 2021

Description:

This PR is based off of #18106

I looked into migrating the piwikApi service, but since it's API is dependent on the angular library, it seemed too much work to create a different implementation, then an adapter for existing angularjs code. Instead I opted to expose the jqXHR object from jQuery in AjaxHelper, which gives it a promise API. New code in Vue will use this API, old jquery specific code can use the callback version of ajaxHelper, and old angularjs code can use piwikApi until Matomo 5. This seemed like the simplest solution and should have no BC breaks.

I also looked at using browser native fetch() function w/ a polyfill (using github's fetch polyfill) in AjaxHelper so we wouldn't have to use jQuery. The API, unfortunately, is too different from jQuery's, so it would be very difficult to provide BC. By Matomo 5, it could be possible to switch the implementation away from jQuery if desired.

New Vue code should use ajaxHelper's promise API.

DOM Sanitization

Also added a new library to sanitize HTML when migrating uses of ng-bind-html since Vue doesn't include one by deafult. Using https://github.com/cure53/DOMPurify, tested with ExampleVue, and it works. ng-bind-html/v-html should ideally not be used, but that would be a large undertaking.

image

image

Note: jump in size is due to dom sanitizer. Unavoidable, but when angularjs is removed, the file size will go down.

Review

@diosmosis diosmosis added the Needs Review PRs that need a code review label Oct 8, 2021
@diosmosis diosmosis added this to the 4.6.0 milestone Oct 8, 2021
@diosmosis
Copy link
Member Author

Also migrated piwik-helper.spec.js + renamed piwik => matomo in as many places as possible in new code.

@diosmosis
Copy link
Member Author

Adding DOM sanitizer library as well so future PRs can just use it.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Looks good to merge @diosmosis . Left a comment to maybe check if it makes sense to print an output in a command.

plugins/CoreVue/Commands/Build.php Show resolved Hide resolved
@diosmosis diosmosis merged commit b5e591c into 4.x-dev Oct 13, 2021
@diosmosis diosmosis deleted the vue-piwik-api2 branch October 13, 2021 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants