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

Disabled localStorage will immediately throw error #25

Closed
yumauri opened this issue Jul 28, 2021 · 5 comments
Closed

Disabled localStorage will immediately throw error #25

yumauri opened this issue Jul 28, 2021 · 5 comments
Labels
help wanted Extra attention is needed rfc Request for comments

Comments

@yumauri
Copy link
Owner

yumauri commented Jul 28, 2021

This line will immediately fail if localStorage is disabled

typeof localStorage !== 'undefined'

In Chrome:
image

In Safari on iOS:
image

Same applies to sessionStorage as well.

@yumauri
Copy link
Owner Author

yumauri commented Jul 28, 2021

Actually, I'm not sure what correct behaviour should be in that case:

  1. Leave this as it is now — runtime error when calling persist function.
  2. Ignore error and behave like local/session storage doesn't exist at all (like in node environment).
  3. Trigger fail event once, when calling persist function. In that case, if fail event will be participant in any logic after calling persist function — this logic will not work, because connections with fail event are not created yet.
  4. Trigger fail event every read/write attempt.

Any item, except 1 — is breaking change, because some code may already depends on runtime error throwing.

@yumauri yumauri added help wanted Extra attention is needed rfc Request for comments labels Jul 28, 2021
@yumauri
Copy link
Owner Author

yumauri commented Jul 28, 2021

Maybe extend adapters with optional function supports (or smth like that)

import { persist, supports } from 'effector-storage/local'

const reason = supports()
if (reason != null) {
  console.log('localStorage is not supported:', reason)
}

And persist will behave like item 2↑ in case adapter is not supported in this particular environment.

@zerobias
Copy link

Is this will work in that way?

function isLSDefined() {
  try {
    return typeof localStorage !== 'undefined'
  } catch (err) {
    return false
  }
}

Any item, except 1 — is breaking change, because some code may already depends on runtime error throwing.

Usually you might imagine code which rely on any throwable case and you'll unable to add even new properties

try {
  persist().anyProperty.toString()
} catch {
  // business logic goes here
  return 'ok'
}
return 'damn effector-storage 😠'

@yumauri
Copy link
Owner Author

yumauri commented Jul 28, 2021

Yes, it is possible to catch this error, and go by 2 way — behave like there is no localStorage at all.

I thought about smth like this, maybe:

function supports() {
  try {
    if (typeof localStorage === 'undefined') {
      return new Error('localStorage is undefined')
    }
  } catch (error) {
    return error
  }
}

Function, which could get the reason, without throwing error.
Inside persist I can use it and ignore all errors (use nil adapter with no-op behaviour) and user could call supports manually, if it is needed.

@yumauri
Copy link
Owner Author

yumauri commented Sep 28, 2021

Fixed in version 4.4.0

@yumauri yumauri mentioned this issue Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed rfc Request for comments
Projects
None yet
Development

No branches or pull requests

2 participants