Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Direct localStorage/ sessionStorage access prevents usage from Worker context or Safari Private tab #44

Open
Tombarr opened this issue Apr 9, 2023 · 8 comments
Labels
enhancement New feature or request ga4

Comments

@Tombarr
Copy link

Tombarr commented Apr 9, 2023

WorkerGlobalScope (which ServiceWorkerGlobalScope inherits from) does not include localStorage or sessionStorage, but using @minimal-analytics@gav4 in a ServiceWorker context throws a ReferenceError because of this hard dependency. It also throws a QuotaExceededError in a Safari Private tab when attempting to call setItem.

Instead, using a wrapper like storage-factory would prevent these runtime errors. Developers in a Worker context would then need to manually set the Client ID cid value, i.e. by copying the clientId value on the main thread between localStorage and indexedDB.

@jahilldev jahilldev added enhancement New feature or request ga4 labels Apr 9, 2023
@jahilldev
Copy link
Owner

Hi @Tombarr,

Thanks for reaching out! Interesting use case, you have here.

Adjusting the ga4 script to support use in non main thread contexts should be fairly trivial.

Feel free to open a PR, all contributions welcome 👍

@Tombarr
Copy link
Author

Tombarr commented Apr 11, 2023

Thanks @jahilldev, working on changes here: main...Tombarr:minimal-analytics:main

Currently getting stuck on issues with yarn dependencies & sub-packages. I'll keep trying, but open to tips. I don't want to pollute your repo, maybe it's easier to keep package versions locked since you'll still have to build & publish on npm anyway?

lerna ERR! yarn exited 1 in '@minimal-analytics/shared'
lerna WARN complete Waiting for 2 child processes to exit. CTRL-C to exit immediately.
npm ERR! code 1
npm ERR! path /Documents/GitHub/minimal-analytics
npm ERR! command failed
npm ERR! command sh -c lerna exec -- yarn
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@minimal-analytics%2fshared - Not found
npm ERR! 404 
npm ERR! 404  '@minimal-analytics/[email protected]' is not in this registry.
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

@jahilldev
Copy link
Owner

Hey @Tombarr,

Ahh yeh, so I should really create a contributors doc..

The package @minimal-analytics/shared isn't published to the registry, it's only used during development.

You normally come across this issue when lerna isn't be run at any point during install. To fix this (I think), you'll need to ensure you're using yarn (for the workspaces) and do the following:

  • Run yarn in the root (this should use lerna exec -- yarn)
  • Run yarn build in the root (this should run lerna bootstrap --no-ci in the pre-build step)

If you're still having issues, let me know, and I'll checkout a fresh copy and put together a CONTRIBUTING.md doc.

Thanks for helping out! 👍

@Tombarr
Copy link
Author

Tombarr commented Apr 13, 2023

Thanks for the tip @jahilldev, I'm able to run tests on the shared package, Now I'm scratching my head at why the local @minimal-analytics@shared package isn't getting imported while running tests.

Once I get past this, I'll finalize my changes and put up a PR. I'll leave the package number updates up to you, then I'll get to work on #43 which should be a much smaller change.

tests/index.test.ts:1:51 - error TS2307: Cannot find module '@minimal-analytics/shared' or its corresponding type declarations.

1 import { clientKey, counterKey, sessionKey } from '@minimal-analytics/shared';

@jahilldev
Copy link
Owner

Hey @Tombarr,

This is great work! Let me know if you're still having issues. I'll Pull your branch and take a look if so 👌

@Tombarr
Copy link
Author

Tombarr commented Apr 15, 2023

Hey @jahilldev, almost done! I got the local package setup working and I'm able to build & test the ga4 package. I added a bunch more tests and an additional navigator.sendBeacon fallback to use fetch when XHR isn't available (i.e. ServiceWorker scope). The only issue now seems to be with the first visit flag. My hunch is it's an issue with mocks, but I'm working to figure out and fix.

BTW, any concerns with the package size? With the fallbacks, index.js not GZipped is up from 6,610 to 9,264 bytes (+40%), which might change the topline number in the README from ~2kb to ~3kb GZipped. On the plus side, once complete it should work in pretty much any browser scope (main thread, Web Worker & ServiceWorker).

@Tombarr
Copy link
Author

Tombarr commented Apr 18, 2023

@jahilldev Should be all set on PR #45. The first attempt failed CI due to unrelated build issues, looks like the second hasn't run yet but it should be good to go. I'm doing some final UAT with the build in my app and not seeing any issues, but understand if you want to run more tests since the changes are significant.

@jahilldev
Copy link
Owner

jahilldev commented Apr 19, 2023

Hey @Tombarr,

Great! I'm travelling for work over the next few days but I'll try and have a thorough look when I get time / when I'm back. I've had a brief look at your PR (good work!), and I think we have a few opportunities to reduce the distributable size.

On a side note, I'd be really interested to know how you found contributing, any specific issues that you feel would've been useful in a document somewhere, etc.

Thanks again for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ga4
Projects
None yet
Development

No branches or pull requests

2 participants