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

add optional chaining in response to sentry logs #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nosovk
Copy link

@nosovk nosovk commented Dec 10, 2024

Time to time there are errors in sentry:

Cannot read properties of null (reading 'getItem')
{0D3109ED-CF45-48B6-ADE9-A2DAA6C0B479}

@MacFJA
Copy link
Owner

MacFJA commented Dec 10, 2024

The storage parameter is required and can't be null as it must implement the interface StorageInterface

StorageInterface interface declaration
/**
 * Storage interface
 */
export interface StorageInterface<T> {
  /**
   * Get a value from the storage.
   *
   * If the value doesn't exist in the storage, `null` should be returned.
   * This method MUST be synchronous.
   * @param key The key/name of the value to retrieve
   */
  getValue(key: string): T | null

  /**
   * Save a value in the storage.
   * @param key The key/name of the value to save
   * @param value The value to save
   */
  setValue(key: string, value: T): void

  /**
   * Remove a value from the storage
   * @param key The key/name of the value to remove
   */
  deleteValue(key: string): void
}

To add the optional chaining to make code works, indicate that the compiler didn't do its job and allowed an invalid value.

Furthermore, the storage can't be defined later, so the nothing will ever be persisted in this case


If you need to conditionally set a storage, and need a "fallback" storage, you can use the createNoopStorage function (it will create a storage that don't persist anything)
(The createNoopStorage is used when the requested storage is not available, like trying to use a localStorage on a Node.js server)

@nosovk
Copy link
Author

nosovk commented Dec 11, 2024

Hm, I was using library within createLocalStorage, but because it's missing in some environments we got crashes in sentry. If there is an option to pass noop as a fallback it would be nice. Now, without that optional chaining it just throws exception and page stops loading. I think that not persisting data is better then unhandled exception.

@MacFJA
Copy link
Owner

MacFJA commented Dec 11, 2024

I was using library within createLocalStorage, but because it's missing in some environments we got crashes in sentry.

The issue seem to be here instead.

The createLocalStorage should fallback in noopStorage if the localStorage is not available: https://github.com/MacFJA/svelte-persistent-store/blob/main/src/core.ts#L267

Do you have more details about the environment that create the crashes ?

@nosovk
Copy link
Author

nosovk commented Dec 15, 2024

It was safari mobile, I think it was private session in it. It seems that before latest releases localStorage was or absent, or with 0 size available.
https://www.reddit.com/r/javascript/comments/2z06aq/local_storage_is_not_supported_with_safari_in/

@nosovk
Copy link
Author

nosovk commented Dec 15, 2024

Like this https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate returning 0 for local storage.

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