feat(routeChangeComplete events): set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values #72

carolineBda · 2022-02-24T10:19:56Z

Inside a next and react app, if we push event to matomo on a useEffect, after a routeChange, the values sent are the old one as currently we set the values on the routeChangeComplete.
This PR allow to set the needed values on the routeChangeStart so they can be used early

…eChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values

codecov-commenter · 2022-02-24T10:22:03Z

Codecov Report

Merging #72 (82f05c1) into master (92b6060) will not change coverage.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   85.00%   85.00%           
=======================================
  Files           1        1           
  Lines          60       60           
  Branches       16       16           
=======================================
  Hits           51       51           
  Misses          1        1           
  Partials        8        8

Impacted Files	Coverage Δ
src/index.ts	`85.00% <90.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92b6060...82f05c1. Read the comment docs.

m-maillot

Top la PR !
J'ai détecté ce que je pense être un bug sur les URLs exclues avec l'envoie d'un event mais le bug était déjà présent avant :)

m-maillot · 2022-02-24T15:24:31Z

src/index.ts

@@ -80,35 +80,37 @@ export function init({
  }
  previousPath = location.pathname;

+  Router.events.on("routeChangeStart", (path: string): void => {
+    if (isExcludedUrl(path, excludeUrlsPatterns)) return;


Quand le path est à exclure, cela signifie que l'on ne met pas à jour les données de matomo.
Je pense que ce if n'est pas à faire dans ce cas précis. On va avoir un comportement étrange quand on va envoyer un event à motomo car il va se baser sur l'ancienne URL vu que l'on n'aura pas mis à jour les données.

effctivement. c'était comme ça avant donc j'imagine que ce choix est conscient ?

oui, cela fait référence aux url qu'on exclue du suivi matomo. Or, là où je trouve la chose tricky, c'est si on passe d'une url exclue à une url non exclue, cela ne settera pas correcment le referrerUrl, non ?

ca mettra la derniere url non blacklistée en referrer oui;
ca me parait correct

m-maillot · 2022-02-24T15:37:27Z

src/__tests__/matomo.test.ts

@@ -77,6 +87,43 @@ describe("push", () => {
  });
 });

+describe("router.routeChangeStart event", () => {


Top les tests 👍 (sans snapshot, la vie est plus folle 🎉 )

franchement ouai 😆

yes c'est niquel ça !

peut-être tester avec la logique d'exclusion d'url aussi pour être sûr que ça ne fasse pas des trucs bizarres

maxgfr · 2022-02-25T09:43:09Z

src/index.ts

@@ -80,35 +80,37 @@ export function init({
  }
  previousPath = location.pathname;

+  Router.events.on("routeChangeStart", (path: string): void => {
+    if (isExcludedUrl(path, excludeUrlsPatterns)) return;


oui, cela fait référence aux url qu'on exclue du suivi matomo. Or, là où je trouve la chose tricky, c'est si on passe d'une url exclue à une url non exclue, cela ne settera pas correcment le referrerUrl, non ?

maxgfr · 2022-02-25T09:44:19Z

src/__tests__/matomo.test.ts

@@ -77,6 +87,43 @@ describe("push", () => {
  });
 });

+describe("router.routeChangeStart event", () => {


yes c'est niquel ça !

peut-être tester avec la logique d'exclusion d'url aussi pour être sûr que ça ne fasse pas des trucs bizarres

revolunet · 2022-02-25T12:46:00Z

Merci pour les corrections !
Est-ce qu'on pousserait pas ca dans une branche alpha pour tester le package npm ?

carolineBda · 2022-02-28T15:09:23Z

Merci pour les corrections ! Est-ce qu'on pousserait pas ca dans une branche alpha pour tester le package npm ?

Avec plaisir. Je peux faire ça depuis ma PR ?
Juste pour info on a déjà fait le même changement sur la lib qu'on utilise dans le CDTN et ça fonctionne 👍 https://github.com/SocialGouv/code-du-travail-numerique/pull/4254/files

i-am-the-slime · 2022-03-10T11:07:35Z

I'd love for this to get merged and released.

revolunet · 2022-03-10T11:17:55Z

lets do this

# [1.3.0](v1.2.2...v1.3.0) (2022-03-10) ### Features * **routeChangeComplete events:** set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values ([#72](#72)) ([52f2bbc](52f2bbc))

github-actions · 2022-03-10T11:20:22Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

carolineBda added 2 commits February 24, 2022 10:40

feat(routeChangeComplete events): set referrerUrl & customUrl on rout…

8dafa0b

…eChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values

Trigger notification

82f05c1

carolineBda mentioned this pull request Feb 24, 2022

Mauvais url dans les événements de tracking SocialGouv/code-du-travail-numerique#4232

Closed

m-maillot approved these changes Feb 24, 2022

View reviewed changes

carolineBda marked this pull request as ready for review February 25, 2022 08:20

carolineBda requested a review from revolunet February 25, 2022 08:20

maxgfr approved these changes Feb 25, 2022

View reviewed changes

revolunet approved these changes Feb 25, 2022

View reviewed changes

revolunet merged commit 52f2bbc into master Mar 10, 2022

github-actions bot added the released label Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(routeChangeComplete events): set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values #72

feat(routeChangeComplete events): set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values #72

carolineBda commented Feb 24, 2022

codecov-commenter commented Feb 24, 2022

m-maillot left a comment

m-maillot Feb 24, 2022

carolineBda Feb 25, 2022

maxgfr Feb 25, 2022

revolunet Feb 25, 2022

m-maillot Feb 24, 2022

carolineBda Feb 24, 2022

maxgfr Feb 25, 2022

maxgfr Feb 25, 2022

maxgfr Feb 25, 2022

revolunet commented Feb 25, 2022

carolineBda commented Feb 28, 2022

i-am-the-slime commented Mar 10, 2022

revolunet commented Mar 10, 2022

github-actions bot commented Mar 10, 2022

feat(routeChangeComplete events): set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values #72

feat(routeChangeComplete events): set referrerUrl and customUrl on routeChangeStart so it can matomo events can be used early on page load (e.g. in a react useEffect) with the correct values #72

Conversation

carolineBda commented Feb 24, 2022

codecov-commenter commented Feb 24, 2022

Codecov Report

m-maillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revolunet commented Feb 25, 2022

carolineBda commented Feb 28, 2022

i-am-the-slime commented Mar 10, 2022

revolunet commented Mar 10, 2022

github-actions bot commented Mar 10, 2022