derrickreimer / fathom-client

A Fathom Analytics library to help with client-side routing
http://npm.im/fathom-client
MIT License
177 stars 18 forks source link

Unload function? #55

Closed subvertallchris closed 4 months ago

subvertallchris commented 6 months ago

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!

derrickreimer commented 6 months ago

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]);
subvertallchris commented 6 months ago

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:

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.

derrickreimer commented 6 months ago

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?

derrickreimer commented 4 months ago

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