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

Check for localStorage support #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

elzii
Copy link

@elzii elzii commented Apr 1, 2022

Adds a function called checkLocalStorageSupport before invoking localStorage.getItem

Description

In some environments, for example: Figma plugins that are entirely inlined data:uri blobs, localStorage is not useable on the parent frame/window object. This first checks support, and allows plausible tracker to be used in some non-traditional contexts.

Should not introduce any breaking changes. Two unit tests are added as well.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@elzii
Copy link
Author

elzii commented Apr 1, 2022

This should probably be pulled out of sendEvent scope and only checked once on instantiation. Larger refactor that I can commit, but want to get author's eyes on this first as proof of concept and understanding the reasoning behind.

Example of error thrown on invoking plausible presently without this check:
image

@ukutaht
Copy link
Contributor

ukutaht commented Apr 4, 2022

Thanks!

The solution we went with in the main codebase is using try-catch when attempting to read from localStorage: https://github.com/plausible/analytics/blob/d40faf6ec796eee23fcd8525d5b96d7623a05702/tracker/src/plausible.js#L38

What I like about this approach is the brevity of it. We try to keep all of our JS as lightweight as possible and the try catch doesn't add much weight.

What do you think about using a similar approach here?

@elzii
Copy link
Author

elzii commented Apr 21, 2022

even more elegant. i like it! thanks for adding. you just opened up plausible to all Figma plugins FYI by doing so :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants