@diosmosis opened this Pull Request on October 21st 2021 Member

Description:

This PR is based off of https://github.com/matomo-org/matomo/pull/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 commented on November 1st 2021 Member

Not ready for review yet, need to re-work the URL handling.

@diosmosis commented on November 2nd 2021 Member

@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...

@diosmosis commented on November 4th 2021 Member

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

@diosmosis commented on November 4th 2021 Member

@sgiehl FYI added related documentation for the store pattern and dealing with the URL here: https://github.com/matomo-org/developer-documentation/pull/581

@sgiehl commented on November 4th 2021 Member

@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 commented on November 5th 2021 Member

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

@tsteur commented on November 5th 2021 Member

@diosmosis I reckon it's good to merge 👍

This Pull Request was closed on November 5th 2021
Powered by GitHub Issue Mirror