-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Comments
Hi @Tombarr, Thanks for reaching out! Interesting use case, you have here. Adjusting the Feel free to open a PR, all contributions welcome 👍 |
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
|
Hey @Tombarr, Ahh yeh, so I should really create a contributors doc.. The package 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
If you're still having issues, let me know, and I'll checkout a fresh copy and put together a Thanks for helping out! 👍 |
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 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.
|
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 👌 |
Hey @jahilldev, almost done! I got the local package setup working and I'm able to build & test the BTW, any concerns with the package size? With the fallbacks, |
@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. |
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! |
WorkerGlobalScope
(whichServiceWorkerGlobalScope
inherits from) does not includelocalStorage
orsessionStorage
, but using@minimal-analytics@gav4
in a ServiceWorker context throws aReferenceError
because of this hard dependency. It also throws aQuotaExceededError
in a Safari Private tab when attempting to callsetItem
.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 theclientId
value on the main thread betweenlocalStorage
andindexedDB
.The text was updated successfully, but these errors were encountered: