fathom-client

fathom-client copied to clipboard

Hello! I'm in the process of adding Fathom to a multi-tenant platform. We'll be dynamically swapping out Fathom Site IDs based on route so our users can get nice analytics for their pages. This means that as route changes, we'll be calling load again. Looking at the code, it looks like we might run into trouble because of this:

  let tracker = document.createElement('script');

  let firstScript =
    document.getElementsByTagName('script')[0] ||
    document.querySelector('body');

Each call to load will add a new script element but firstScript will grab the first, which may or may not be the new one we created. If a user clicks between enough stores, we could potentially see this as a memory leak since it is adding elements to the document that are never removed.

First off: is my reading of this correct? If I'm off the mark here, please cut me off and let me know. :-)

As far as I can tell, I can manually perform a cleanup:

function unload(() {
  window.__fathomClientQueue = [];
  const tracker = document.getElementById('fathom-script');
  if (tracker) {
    tracker.remove();
  }
}

Or in a React hook:

  React.useEffect(() => {
    load(fathomSiteId, {
      auto: false,
    });

    return () => {
      if ('__fathomClientQueue' in window) {
        window.__fathomClientQueue = [];
        const tracker = document.getElementById('fathom-script');
        if (tracker) {
          tracker.remove();
        }
      }
    };
  }, [fathomSiteId]);

This is quite brittle and dependent on your current implementation, though.

If I am correct about this unloading process, would you accept a PR that adds this behavior? I'd also like to make the Tracker ID configurable and talk about ways to support multiple clients loaded simultaneously, which would let someone like me have one for their entire platform and a second for a tenant's site.

Thank you for your work!

Hey @subvertallchris! Great question here. We actually do something similar in our application by basically swapping the site ID before executing a trackPageview call. You can use the setSiteId function for this (simplified example):

import { load, setSiteId, trackPageview } from "fathom-client";

// Load Fathom script on initial render
useEffect(() => {
  load(fathomSiteId);
}, []);

// Anytime the site id changes...
useEffect(() => {
  setSiteId(fathomSiteId);
  trackPageview();
}, [fathomSiteId]);

Hey @derrickreimer thanks for the advice! Using setSite does help that piece, thank you. I still wind up with many instances of the script loaded into the page. For instance:

• Not every store has the Fathom feature enabled. For those stores, we don't load the script at all because we don't have a value we can feed to load. On those pages, the container returns null.
• Imagine a user browses to three pages -- Page1 with Fathom, Page2 without, Page3 with -- the container will mount, unmount, and mount again, calling load twice across it on pages 1 and 3
• In those cases, we'll wind up with multiple instances of the script because it cannot be reused

Beyond that, one will wind up with two instances in dev mode when React Strict mode is enabled. Adding the unload behavior is useful for that if nothing else.

My only hesitation about an unload function is that it wouldn't actually remove Fathom from the page (only its <script> tag), unless we reached behind the curtain and tried to remove any objects/event listeners/etc. that Fathom installs on the page when it gets loaded. (Those implementation details of their script are not really public).

That said, it would be nice to make load idempotent, so that duplicate calls won't result in multiple instances of the script getting injected (which would also solve the Strict Mode issue also mentioned in #54). Seem reasonable?

Closing this one for now, since the load function is now idempotent (so duplicate calls will not trigger multiple injections of the script tag).