PKG -- [fcl] export LOCAL_STORAGE const

[austinkline]

Currently there is a way to inject the use of local storage into fcl so that logins persist across tabs, but the const that enables this easily (no custom code by the consumer) isn't exported

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: bd2a27b111398a96e510e37ebcb604be161f8271

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@onflow/fcl	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[gregsantos]

thanks @austinkline This is included as a reference and prob should be removed rather than exported. You are free to use "config.fcl.storage" to overide the default with localStorage or other strategy

[justinbarry]

@austinkline Is the above sufficient for your purposes?

[austinkline]

@gregsantos @justinbarry

Hello, I had given config.fcl.storage being set to localStorage a try previously, and did not see anything change or get put into my localStorage for the user itself.

When I had looked into this before, it appeared as though that config item is actually looking for an object like the one that this PR would export, not a string like localStorage

Here's what I had found when I looked into this again just now: https://github.com/austinkline/fcl-js/blob/de47af647a5bdad154a2d83e2ea2260ab54f0c60/packages/fcl/src/current-user/index.js#L56

const storage = await config.first(["fcl.storage", "fcl.storage.default"])
    if (storage.can) {
      const user = await getStoredUser(storage)
      if (notExpired(user)) ctx.merge(user)
    }

if you look into getStoredUser you will see that it uses the value of fcl.storage or fcl.storage.default as an object and attempts to call get on the user. https://github.com/austinkline/fcl-js/blob/de47af647a5bdad154a2d83e2ea2260ab54f0c60/packages/fcl/src/current-user/index.js#L32

const getStoredUser = async storage => {
  const fallback = JSON.parse(DATA)
  const stored = await storage.get(NAME) // this part right here!!!
  if (stored != null && fallback["f_vsn"] !== stored["f_vsn"]) {
    storage.removeItem(NAME)
    return fallback
  }
  return stored || fallback
}

Perhaps I'm missing something here? If we supply a default and we already have an implemented version to swap over to a version that persists across tabs, then what's the reasoning behind opting to instead delete it? Finding this took quite a bit of digging, if fcl is supposed to make lots of this out of box, it feels like this would definitely fit that mission given how common it would be used in marketplaces where users open many tabs. Curious your thoughts on this. Thanks!

[gregsantos]

Thanks @austinkline The idea is that you can override fcl.storage.default (session) via config.fcl.storage with whatever you like. So you can inject your storage implementation like so:

const isServerSide = () => typeof window === "undefined"
const LOCAL_STORAGE = {
  can: !isServerSide(),
  get: async key => JSON.parse(localStorage.getItem(key)),
  put: async (key, value) => localStorage.setItem(key, JSON.stringify(value)),
}

config({ "fcl.storage": LOCAL_STORAGE })

This will use localStorage instead of sessionStorage Does that make sense?

[austinkline]

Thanks @austinkline The idea is that you can override fcl.storage.default (session) via config.fcl.storage with whatever you like. So you can inject your storage implementation like so:

const isServerSide = () => typeof window === "undefined"
const LOCAL_STORAGE = {
  can: !isServerSide(),
  get: async key => JSON.parse(localStorage.getItem(key)),
  put: async (key, value) => localStorage.setItem(key, JSON.stringify(value)),
}

config({ "fcl.storage": LOCAL_STORAGE })

This will use localStorage instead of sessionStorage Does that make sense?

Makes sense, yes. My confusion is why folks consuming fcl-js have to implement/dig into all this themselves when there's already a ready-made solution available. Generally speaking, I'd argue that exporting some version of LOCAL_STORAGE tracks extremely well with the purpose that fcl-js was made for, it would make my life and other consumers' lives easier.

If all dapps are expected to make their own implementation of this, isn't it more likely they'll copy exactly what you've put here and put it in their application? And if that's the case, isn't it cleaner/easier to maintain for fcl-js to export it so that if you ever need to adjust it, any migration plan isn't requiring nearly as much coordination? You'd be able to just ship a new version of the implementation and dapps can just upgrade fcl-js and be happy

If the concern is exporting the variable itself, what about permitting a string value in that config variable and then detecting/translating it into the LOCAL_STORAGE object when the value is read here: https://github.com/austinkline/fcl-js/blob/de47af647a5bdad154a2d83e2ea2260ab54f0c60/packages/fcl/src/current-user/index.js#L56

If that also isn't okay, I'll drop it. But as a consumer of this package, either of these two things would be much easier than digging through the actual code and figuring out how storage is working and then implementing my own version of it

Lastly, if this isn't sufficient justification, where can I add something in docs so that folks can at least discover this as an option instead of having to clone and comb through the code to find this?

[considine]

@gregsantos @austinkline chiming in, albeit a bit late. The syntactic sugar suggested here would save my team a bit of copy pasta, any reason why consumers should use that snippet as opposed to extending the lib?

[justinbarry]

@austinkline Thank you for your PR. Couple things and I'll merge it down:

1. Add a changeset file. I believe it would be a patch here. This updates the relevant changelog files and keeps the machinery of progress forward.
2. export the other storage options for consistency.

[austinkline]

@austinkline Thank you for your PR. Couple things and I'll merge it down:

1. Add a changeset file. I believe it would be a patch here. This updates the relevant changelog files and keeps the machinery of progress forward.
2. export the other storage options for consistency.

Thanks @justinbarry! Went ahead and updated per your request, let me know if I've missed anything and I'll be sure to give it another go 🙏

[justinbarry]

@austinkline How are you planning to access these values? Importing the file directly?

[austinkline]

@austinkline How are you planning to access these values? Importing the file directly?

Good question, I have no strong feelings here. Is there an ideal place to put exported values such as these that people will export? Given that these are unlikely to be used by all folks consuming fcl-js, I am fine with it being on the file so long as that doesn't get messed up with importing things we might not want to. I'm much more concerned with the ability to import them as opposed to where they should live, happy to move them somewhere else if need be

[gregsantos]

I am thinking maybe we make this a config option for now rather than export these. useLocalStorage: boolean

[jribbink]

superseded by https://github.com/onflow/fcl-js/pull/1782