benjaminhoffman / gatsby-plugin-segment-js

Gatsby plugin for segment.com's analytic.js snippet
https://www.npmjs.com/package/gatsby-plugin-segment-js
MIT License
40 stars 28 forks source link

`page()` does not capture correct path if site uses `canonical` link tag #13

Open andycmaj opened 5 years ago

andycmaj commented 5 years ago

my site uses a link rel="canonical" tag to support various crawlers/social share/etc. I do this with react-helmet.

calling analytics.page() on onRouteUpdated doesn't work in this case, because this fires before the react component that renders the meta tag is re-rendered.

so at the time when analytics.js tries to figure out what hte current page path is, it looks at the canonical tag and sees <link data-react-helmet="true" rel="canonical" href="https://todayilearned.io/">.

so page() path is set to /.

ScreenCapture at Thu Mar 14 23:56:00 PDT 2019 ScreenCapture at Thu Mar 14 23:56:38 PDT 2019

the fix could be to use the location.pathname passed to the onRouteUpdate hook, as it's the correct route at that point:

ScreenCapture at Thu Mar 14 23:57:15 PDT 2019

segment.js does allow passing in the path manually: https://segment.com/docs/sources/website/analytics.js/#default-properties

benjaminhoffman commented 5 years ago

🤔are you sure analytics.js looks at the canonical tag to determine the current page path? can you link to the code where it does this?

if you're right, that would be super interesting. and then, maybe you can open a PR for this fix? although, i'd first like to be extra sure that your hypothesis is correct.

i wonder if another solution could be to wrap the call in a setTimeout with 0ms value to give the component time to re-render.

andycmaj commented 5 years ago

as for the analytics.js code, as you can see i was debugging the minified code in the browser, but i'm 90% sure that this is the actual code that does this:

function pageDefaults() {
  return {
    path: canonicalPath(),
    referrer: document.referrer,
    search: location.search,
    title: document.title,
    url: canonicalUrl(location.search)
  };
}

i was thinking of the setTimeout idea as well, but onRouteUpdated gives you the new path as a param, so that woudl be authoritative and deterministic...

but actually, thinking about this a bit more, the canonical thing isn't really the problem here... the problem is the timing of the lifecycle event.

onRouteUpdated doesn't give you the updated title, other params either. these actually also turn out to be stale using page(). the underlying root cause here is the fact that this lifecycle event happens before your root component re-renders.

so whatever you log here that's derived from current DOM or browser state is going to be wrong.

i ended up not using onRouteUpdated for this reason. this may be a hack, but seems more deterministic:

export const wrapPageElement = ({ element, props }) => {
  // props provide same data to Layout as Page element will get
  // including location, data, etc - you don't need to pass it
  const {
    location: { pathname, search, href },
    pageContext: {
      title = 'Today I Learned',
      description = 'Stuff I learned one day',
    },
  } = props;

  if (!window.analytics || typeof window.analytics.page !== 'function') {
    console.warn('Unable to locate analytics.js');
    return;
  }
  window.analytics.page({
    path: pathname,
    referrer: document.referrer,
    search: search,
    title: title,
    url: href,
  });

  return element;
};

of course this requires you to pass title, etc. as pageContext when you build your pages from data.

thoughts?

aileen commented 5 years ago

I was also running into this lifecycle issue. Unfortunately using pageContext wasn't an option for my use case, so I actually ended up using a little timeOut of 500ms, which works like a charm!

benjaminhoffman commented 4 years ago

hey @andycmaj ... i think this is now solved with this merge... https://github.com/benjaminhoffman/gatsby-plugin-segment-js/pull/30

mind checking by upgrading to v3.3.0? :)

muravitskiy commented 4 years ago

I had the same issue with react-metrics+segment and was able to solve it using <Helmet defer={false} ... option. From the react-helmet docs:

(optional) set to false to not use requestAnimationFrame and instead update the DOM as soon as possible.
        Useful if you want to update the title when the tab is out of focus
benjaminhoffman commented 4 years ago

anyone here know if the following PR would fix this issue? my guess is maybe not but i’m curious your thoughts.

https://github.com/benjaminhoffman/gatsby-plugin-segment-js/pull/32

xtellurian commented 2 years ago

I'm still seeing this issue on 3.7.1