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] migrate comparisons service + component #18193

Merged
merged 45 commits into from Nov 5, 2021
Merged

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Oct 21, 2021

Description:

This PR is based off of #18166.

Note: I checked the comparisons UI tests locally and they passed, but I checked nothing else. So this PR is blocked until the CI issues are dealt with.

Changes:

  • Turned the camelcase eslint rule off since response data does not include camelcase most of the time and it would get tiring to eslint-ignore that each time.
  • Convert comparisons service to comparisons store (see https://v3.vuejs.org/guide/state-management.html). Not implemented exactly as a store would be, but it works and interacts w/ angularjs code correctly.
  • Migrate comparisons component.
  • Fix URL encoding/angularjs issue in reporting menu described here: https://github.com/matomo-org/matomo/pull/18193/files#diff-94a861e734c0bb0ad8fdd4fa85389cb8303f90805917fbdd677f6c47c2b34810R132
  • Fix AjaxHelper.fetch requires some extra parameters to be sent for API requests (module=API&format=json).
  • Fix AjaxHelper.send, jqXHR objects do not follow the promise API exactly, so it's now wrapped in a native Promise.
  • Allow using restrict: 'E' in createAngularJsAdapter for use w/ migrating AngularJS components.
  • Create pattern for migrating $rootScope events (described below).
  • Create pattern for migrating $location usage (described below).
  • Convert comparisons service test file to typescript/vue. Includes the addition of https://github.com/nock/nock as a dependency.

image

Migrating $rootScope events

In angularjs, $rootScope.$emit is used as a way to implement pub/sub throughout the frontend. We will of course not be able to use this mechanism with Vue, so I started replacing it here.

Instead of $rootScope, we are just using new CustomEvent and window.dispatch directly (so we don't need to depend on another library). This functionality is exposed in the Matomo service, via Matomo.on/Matomo.off/Matomo.postEvent.

During the migration, $rootScope.$emit is overridden to use Matomo.postEvent and Matomo.postEvent will use the old $rootScope.$emit. This way, $rootScope.$emit calls will trigger event handlers that still use $rootScope and handlers that use Matomo.on. As will Matomo.postEvent calls. Before Matomo 5 is released, this code will be removed.

Similarly, $broadcast will also call Matomo.postEvent, so Matomo.on can be used w/ events like $locationChangeSuccess.

Migrating use of $location

AngularJS code that uses $location will be replaced with simply directly reading and writing window.location.hash. This is the simplest approach and allows us to keep our custom hash based routing.

The above approach will not work until AngularJS is removed, because $location events, like $locationChangeSuccess, are triggered before changes to the hash/URL are made. So now, if comparisons listens to the hashchange, it will execute it's event handler AFTER others like the reporting page handler, which breaks comparison since the comparison will not be updated at that point. Currently, the ordering works because the comparison event handler to $locationChangeSuccess is added before the reporting page one, simply because the JS file is loaded afterwards.

This PR replaces that system by creating a store for parsed URL information in the MatomoUrl class, then using Vue computed properties to process it. The computed properties are used in Vue components, so when the URL is updated, the specific components that depend on that data automatically notice the change and update. This means event handler order is entirely irrelevant, and there is only one part of the code that parses the URL and one part of the code that uses $locationChangeSuccess. (This is, as I understand it, the accepted way to implement routing in modern state driven UIs (at least in React), where the URL is treated as a piece of application state with derived/computed state at various parts of the application).

For comparisons, this looks like this:

  1. on $locationChangeSuccess, MatomoUrl sets the search and hash of the URL to two ref() properties.
  2. there are a couple computed properties that use those two ref() values to compute a query parameter object (ie, MatomoUrl.parsed.value.period).
  3. there are computed properties in ComparisonStore that use the MatomoUrl.parsed property to parse segment/period comparison information & other information.
  4. the Comparisons.vue component consumes these computed properties and uses them in the template. The template registers with these computed properties, and re-renders the component (or parts of it), when one or more of the properties change.
  5. if the url changes, MatomoUrl will update the values of the two ref() properties mentioned above, and every computed property will automatically be aware of the change, and update.

When angularjs is removed entirely, we can stop listening to $locationChangeSuccess in MatomoUrl, and use hashchange.

Existing angularjs code can still use $location.

Review

@diosmosis diosmosis added this to the 4.6.0 milestone Oct 21, 2021
@diosmosis diosmosis marked this pull request as draft October 21, 2021 02:28
@diosmosis diosmosis added the Needs Review PRs that need a code review label Oct 21, 2021
@diosmosis diosmosis marked this pull request as ready for review October 21, 2021 23:32
result[key].push(value);
} else {
result[key] = value;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Modified this function to handle arrayParam[]=... parameters. It will notice phpArrayParam[complexKey]=... type parameters, but will ignore the key type. (These are used in heatmaps if I recall correctly, so thought I'd mention.)

@diosmosis diosmosis marked this pull request as ready for review November 2, 2021 08:33
@diosmosis diosmosis added the Needs Review PRs that need a code review label Nov 2, 2021
@diosmosis
Copy link
Member Author

@sgiehl (and @peterhashair if you're reviewing the Vue PRs as well), this should be ready for review. It got a lot bigger than I expected, and also includes new patterns for dealing w/ URLs/routing and $rootScope events. It would be good if it got a thorough review, but it is bigger than I expected it would be...

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

I had a look through all the changes. But to be honest I'm not very experienced with type script, so I'm not able to say much about it. maybe @peterhashair can say more on that.

Nevertheless I spent some time clicking through the comparison UI. So far I found some javascript errors popping up in the console when typing a custom compare period:

image

plugins/CoreHome/vue/src/ContentBlock/ContentBlock.vue Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

@sgiehl fixed the last UI tests and fixed the javascript errors you mentioned.

@diosmosis
Copy link
Member Author

@sgiehl FYI added related documentation for the store pattern and dealing with the URL here: matomo-org/developer-documentation#581

@sgiehl
Copy link
Member

sgiehl commented Nov 4, 2021

@diosmosis javascript errors are gone 👍 Guess this one also needs to be updated: https://builds-artifacts.matomo.org/matomo-org/matomo/vue-comparisons/50661/PeriodSelector_custom_comparison.png
Otherwise this seems to work correctly for me. If @peterhashair doesn't have any additional input this should be good to merge I guess.

@diosmosis
Copy link
Member Author

@tsteur do you know if it's ok to merge this or if I should wait a bit more for other reviews?

@tsteur
Copy link
Member

tsteur commented Nov 5, 2021

@diosmosis I reckon it's good to merge 👍

@diosmosis diosmosis merged commit 2b94200 into 4.x-dev Nov 5, 2021
@diosmosis diosmosis deleted the vue-comparisons branch November 5, 2021 05:39
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

3 participants