fix(router): subsequent navigation to the same url should not trigger…

[p-aron]

I am trying to fix #72. I am not sure if my approach is acceptable. I am not sure how PageUrlProvider could be used. location.pathname also contains the pathname without the queryparam.
If my understanding is correct then the previous distinctUntilKeyChanged('urlAfterRedirects') was deadcode because if user navigates to the same url NavigationEnd event is not triggered only NavigationSkipped.

[EmmanuelRoux]

Hi @p-aron and thank you very much

I think this behavior should be opt-in (and it can easily be a non-breaking change).

Maybe it could be introduced as a comparator property in MatomoRouterConfiguration?

[p-aron]

Hello thank you for your response.
I would like to tell you what is my use case. In our application let's say when there is a table with a search bar and pagination we put those information to query parameters. We do not want to track those as seperate page views. Those generates too much page view and those are not really page views.

I am thinking about what you wrote that it should not be a breaking change. I am not sure how my change can be a breaking change.

Do you mean a comparator?: (previous: T, current: T) => boolean function which is passed to distinctUntilChanged?
And it should be optional if it is not then no distinctUntilChanged is used?
or the previous distinctUntilKeyChanged('urlAfterRedirects') should be the default?

[EmmanuelRoux]

I understand the use case and agree with it

I am not sure how my change can be a breaking change.

Your change is breaking, since some urls won't be tracked anymore. This is a change in behavior that some users may not expect.

Do you mean a comparator?: (previous: T, current: T) => boolean function which is passed to distinctUntilChanged?

yes exactly

And it should be optional if it is not then no distinctUntilChanged is used?
or the previous distinctUntilKeyChanged('urlAfterRedirects') should be the default?

If not provided, the default would be a comparator that mimics urlAfterRedirects comparison, something like this:

function compareUrlAfterRedirect(event1, event2) {
return event1.urlAfterRedirects === event2.urlAfterRedirects`;
}

[p-aron]

I understand that it changes the behavior, but it would not stop other's code from compiling or running.

My problem is that distinctUntilKeyChanged('urlAfterRedirects') is a bug in the code; it does not do what it is intended to do. So the change would fix other people's applications.

Is distinctUntilKeyChanged('urlAfterRedirects') a bug in the code or a feature? If it is moved into a config, then people will need to update their code. Are you saying there are people for whom this would not be a fix? Or that we can not be sure?

[EmmanuelRoux]

I understand that it changes the behavior, but it would not stop other's code from compiling or running.

This is a breaking change, because it can make some applications fail to run correctly after that.

My problem is that distinctUntilKeyChanged('urlAfterRedirects') is a bug in the code; it does not do what it is intended to do. So the change would fix other people's applications.

Why is it a bug? I think it does exactly what is was supposed to do. But I understand is is maybe not expected by everyone's apps. That's why making it configurable is a good idea :)

Is distinctUntilKeyChanged('urlAfterRedirects') a bug in the code or a feature? If it is moved into a config, then people will need to update their code.

No, if we keep it as a default behavior (= if not configured explicitly by user), people won’t need to update their code, that’s the point.

Are you saying there are people for whom this would not be a fix? Or that we can not be sure?

Yes, this behavior has never been reported as a bug. Also, some apps may rely on the current behavior and we must not break them.

[p-aron]

Why is it a bug? I think it does exactly what is was supposed to do. But I understand is is maybe not expected by everyone's apps. That's why making it configurable is a good idea :)

Ok I did not know about the onSameUrlNavigation: reload option. So I was wrong when I said it is dead code.

Yes, this behavior has never been reported as a bug. Also, some apps may rely on the current behavior and we must not break them.

I think it is more probable that people did not discover this. For me it is hard to imagine a scenario when somebody relies on tracking page views when modifying query params.

But anyhow I understand your concerns. I will do the necessary modifications.

[EmmanuelRoux]

For me it is hard to imagine a scenario when somebody relies on tracking page views when modifying query params.

Imagine you navigate to /my-page?param=1, then you hard (re)load your browser on /my-page?param=2.
In this case there is no way to avoid an Angular Router navigation, so two page views will be tracked (same problem still exists without query params).

The current behavior matches that and this initial choice for distinct on query params was to be consistent.

FYI, Matomo provides a way to handle that server-side (see FAQ).

Anyway I totally understand and quite agree to what you're saying and maybe the initial choice was not the best, since it is not fixable in all cases on frontend-side.

I was just wondering about not introducing breaking changes too early, but I agree that this topic can be discussed and default behavior may (or should) be updated!

Your PR is a good improvement as it allows users to configure what they want for their specific scenario.

[EmmanuelRoux]

@p-aron I pushed some changes to implement what I was explaining

I now wonder if this is not over-engineered for a relatively simple use-case... but it'll do the job :)

So, the default behavior is unchanged, but it can be easily configured:

provideMatomo( 
    { ... },
    withRouter({
        navigationEndComparator: 'ignoreQueryParams', // <-- This is your initial request
    })
);

And leaving the ability to compare using custom function (e.g. for comparing only some specific query params, or any other processing) :

provideMatomo( 
    { ... },
    withRouter({
        navigationEndComparator: (previous, current) => {
            return /* ...custom comparison logic here */;
        }
    })
);

[EmmanuelRoux]

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

• v6.2.0
• GitHub release

Your semantic-release bot 📦🚀

[p-aron]

@EmmanuelRoux Sorry I couldn't implement it yet. Thanks for implementing.

I now wonder if this is not over-engineered for a relatively simple use-case... but it'll do the job :)

Yes I can not see the use case for providing custom comparator.

But I like I can pass ignoreQueryParams to the config without implementing my logic. I was not thinking about this but it's nice.

[p-aron]

@EmmanuelRoux
Hello one of my coworker notified this solution has a caveat.
I wanted to tell you also.
The current ignoreQueryParams solution will also filter out site searches:
https://matomo.org/faq/reports/tracking-site-search-keywords/

[EmmanuelRoux]

Hi @p-aron, maybe I didn't understand your initial request : I thought it was to ignore query params changes, and that's exactly what ignoreQueryParams option does (and you're right, Matomo tracks site searches using some predefined query params, by default).

So what's your exact need?

• If you want to ignore all query params changes, you can use ignoreQueryParams
• If you only want to ignore some query params but not all, you can provide a custom navigationEndComparator function
• If you want to track site searches independently from router navigation, you can use trackSiteSearch api method (available via injectable MatomoTracker)