feat(utils): allow providing a subscribe method to createJSONStorage util #2539

mhsattarian · 2024-05-03T21:09:21Z

Related Issues or Discussions

Summary

previously for providing subscribe functionality to a custom persist storage, one should've used something like the code below which btw was not documented.

const stringStorage = {
  getItem: (key: string) => {
    return //
  },
  setItem: (key: string, newValue: string) => {
    //
  },
  removeItem: (key: string) => {
    ///
  },
}

const customStorage = createJSONStorage<number>(() => stringStorage)

customStorage.subscribe = (key, callback, initialValue) => {
  /// add listener to detected chanegs then call callback:
  callback()
  return () => {
    /// remove listener
  }
}

it's more convenient to just accept the subscribe method along with getItem, setItem, etc when using createJSONStorage to create a custom persist functionality like the cookies' in the linked discussed. here is an example:

const stringStorage = {
  getItem: (key: string) => {
    return; //
  },
  setItem: (key: string, newValue: string) => {
    //
  },
  removeItem: (key: string) => {
    //
  },
  subscribe(key, callback, initialValue) {
    /// add listener to detected chanegs then call callback:
    callback();
    return () => {
      /// remove listener
    };
  },
};

const customStorage = createJSONStorage<number>(() => stringStorage);

Check List

yarn run prettier for formatting code and docs
provide tests for the added functionality

vercel · 2024-05-03T21:09:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
jotai	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 24, 2024 5:03am

codesandbox-ci · 2024-05-03T21:09:52Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

dai-shi

Thanks for working on this!
The idea seems good. We just need some fixes.
Please take your time.

dai-shi · 2024-05-03T23:21:04Z

src/vanilla/utils/atomWithStorage.ts

@@ -114,6 +129,42 @@ export function createJSONStorage<Value>(
 ): AsyncStorage<Value> | SyncStorage<Value> {
  let lastStr: string | undefined
  let lastValue: Value
+
+  const webStorageSubscribe: Subscribe<Value> = (key, callback) => {


This is still string based, right?

Suggested change

const webStorageSubscribe: Subscribe<Value> = (key, callback) => {

const webStorageSubscribe: Subscribe<string> = (key, callback) => {

before this PR, subscribe method was defined like this:

subscribe?: ( key: string, callback: (value: Value) => void, initialValue: Value, ) => Unsubscribe

and the callback would accept value of type Value. should we change this to accept only strings?

hey @dai-shi. I finally got some time to work on this. I would appreciate your opinion (and advice) on typing issues.

Regarding this code suggestion, webStorageSubscribe was previously typed as Subscribe<Value> and I didn't change the type here. how should we address these typings? Isn't the Value type always a subset of string?

dai-shi · 2024-05-03T23:21:22Z

src/vanilla/utils/atomWithStorage.ts

+    }
+    const storageEventCallback = (e: StorageEvent) => {
+      if (e.storageArea === getStringStorage() && e.key === key) {
+        callback((e.newValue || '') as Value)


This seems wrong:

Suggested change

callback((e.newValue || '') as Value)

callback(e.newValue || '')

dai-shi · 2024-05-03T23:22:24Z

src/vanilla/utils/atomWithStorage.ts

+type SubscribeHandler<Value> = (
+  subscribe: Subscribe<Value>,
+  key: string,
+  callback: (value: Value) => void,
+  initialValue: Value,
+) => Unsubscribe
+


Maybe, we don't need this type alias. (But, not 100% sure.)

would something like this be more acceptable?:

const handleSubscribe = ( subscriber: Subscribe<Value>, ...params: Parameters<Subscribe<Value>> ) => { const [key, callback, initialValue] = params function callbackWithParser(v: Value) { let newValue: Value try { newValue = JSON.parse((v as string) || '') } catch { newValue = initialValue } callback(newValue as Value) } return subscriber(key, callbackWithParser, initialValue) }

Yes, I meant to inline it as it's used just once.

dai-shi · 2024-05-03T23:23:08Z

src/vanilla/utils/atomWithStorage.ts

+    function callbackWithParser(v: Value) {
+      let newValue: Value
+      try {
+        newValue = JSON.parse((v as string) || '')


v must be a string always.

Suggested change

newValue = JSON.parse((v as string) || '')

newValue = JSON.parse(v || '')

What do you suggest to ensure this?

should we make change the generic types to this:

<Value extends string | null>

or should we use something like JSON.parse(String(v) || '')?

No, I mean v must be string type without any hacks/casts. It feels like there are some lies in types.
I can help for typing later. Please work on other comments first.

dai-shi · 2024-05-03T23:24:05Z

src/vanilla/utils/atomWithStorage.ts

-      return () => {
-        window.removeEventListener('storage', storageEventCallback)
-      }
+    if (getStringStorage()?.subscribe) {


AFAIR, this can throw. So, we can't call it here.

I believe the removeEventListener method won't throw unless the given parameters are wrong which is not happening in our case. am I wrong?

and this code is the same as before:

jotai/src/vanilla/utils/atomWithStorage.ts

Line 166 in a4dc985

window.removeEventListener('storage', storageEventCallback)

Oh, my bad. I was thinking of old implementation. We catch it already.

My first comment was actually right. We can't call getStringStorage() on creation. #2581

dai-shi · 2024-05-03T23:26:59Z

src/vanilla/utils/atomWithStorage.ts

+      storage.subscribe = handleSubscribe.bind(
+        null,
+        getStringStorage()!.subscribe as unknown as Subscribe<Value>,
+      )


I don't think we use bind like this in our code base. Please either use an inline function or a higher-order function.
But, restructure will be needed necessary anyway because of the previous comment.

applied a few changes to fix this. I would appreciate a review of them.

dai-shi · 2024-05-15T01:25:42Z

Hope you find time to address this.

github-actions · 2024-05-15T01:28:24Z

Preview in LiveCodes

Latest commit: 38bb47e
Last updated: May 24, 2024 5:02am (UTC)

Playground	Link
React demo	https://livecodes.io?x=id/ATDNPCJA6

See documentations for usage instructions.

mhsattarian · 2024-05-15T14:48:39Z

@dai-shi I would love to.
sorry this is taking some time, I'm a bit busy with family these days and I hope I can find time to work on this in a couple of days.

dai-shi · 2024-05-24T04:54:06Z

@mhsattarian Hey, I just added a commit to refactor. Please review it.

dai-shi · 2024-05-24T04:56:48Z

src/vanilla/utils/atomWithStorage.ts

+    (typeof window !== 'undefined' &&
+      typeof window.addEventListener === 'function' &&
+      ((key, callback) => {
+        if (!(getStringStorage() instanceof window.Storage)) {


It feels like we can simply this, if we are allowed to call getStringStorage in sync.

mhsattarian · 2024-05-25T09:44:00Z

Hey @dai-shi, sorry for the late response.
this looks clean! thank you.

Are we ready for the merge?

dai-shi · 2024-05-25T10:01:10Z

Cool. I will merge it next week before preparing a new patch release.

mhsattarian · 2024-05-25T10:07:37Z

Awesome!

since this PR started from syncing atoms with cookies, I will soon open a new PR for an example in the persistence guide.
Is there anything I should know before that? (I remember you said something about re-structuring the persistence guide)

dai-shi · 2024-05-25T10:20:57Z

I don't remember exactly what I said, but the persistence guide (and other docs) isn't very polished. So, not only adding new things, but also improving the guide entirely would be appreciated.

feat: allow providing a subscribe method to createJSONStorage util

b1f194e

vercel bot deployed to Preview May 3, 2024 21:10 View deployment

dai-shi requested changes May 3, 2024

View reviewed changes

Merge branch 'main' into feat/createJSONStorage-subscribe-method

0657feb

vercel bot deployed to Preview May 15, 2024 01:28 View deployment

refactor: convert handleSubscribe to a HOF

82af4a6

vercel bot deployed to Preview May 22, 2024 21:05 View deployment

dai-shi added 2 commits May 24, 2024 13:12

Merge branch 'main' into feat/createJSONStorage-subscribe-method

640f4ad

refactor

1044dfa

vercel bot deployed to Preview May 24, 2024 04:54 View deployment

dai-shi reviewed May 24, 2024

View reviewed changes

refactor 2

38bb47e

vercel bot deployed to Preview May 24, 2024 05:03 View deployment

dai-shi changed the title ~~feat: allow providing a subscribe method to createJSONStorage util~~ feat(utils): allow providing a subscribe method to createJSONStorage util May 24, 2024

dai-shi added this to the v2.8.2 milestone May 24, 2024

dai-shi approved these changes May 24, 2024

View reviewed changes

dai-shi merged commit 88362ed into pmndrs:main May 27, 2024
37 checks passed

rtritto mentioned this pull request May 28, 2024

createJSONStorage: localStorage is not defined #2581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utils): allow providing a subscribe method to createJSONStorage util #2539

feat(utils): allow providing a subscribe method to createJSONStorage util #2539

mhsattarian commented May 3, 2024

vercel bot commented May 3, 2024 •

edited

codesandbox-ci bot commented May 3, 2024 •

edited

dai-shi left a comment

dai-shi May 3, 2024

mhsattarian May 6, 2024

mhsattarian May 22, 2024

dai-shi May 3, 2024

dai-shi May 3, 2024

mhsattarian May 6, 2024

dai-shi May 6, 2024

dai-shi May 3, 2024

mhsattarian May 6, 2024

dai-shi May 6, 2024

dai-shi May 3, 2024

mhsattarian May 22, 2024 •

edited

dai-shi May 24, 2024

dai-shi May 29, 2024

dai-shi May 3, 2024

mhsattarian May 22, 2024

dai-shi commented May 15, 2024

github-actions bot commented May 15, 2024 •

edited

mhsattarian commented May 15, 2024

dai-shi commented May 24, 2024

dai-shi May 24, 2024

mhsattarian commented May 25, 2024

dai-shi commented May 25, 2024

mhsattarian commented May 25, 2024

dai-shi commented May 25, 2024

	const webStorageSubscribe: Subscribe<Value> = (key, callback) => {
	const webStorageSubscribe: Subscribe<string> = (key, callback) => {

	callback((e.newValue \|\| '') as Value)
	callback(e.newValue \|\| '')

	newValue = JSON.parse((v as string) \|\| '')
	newValue = JSON.parse(v \|\| '')

feat(utils): allow providing a subscribe method to createJSONStorage util #2539

feat(utils): allow providing a subscribe method to createJSONStorage util #2539

Conversation

mhsattarian commented May 3, 2024

Related Issues or Discussions

Summary

Check List

vercel bot commented May 3, 2024 • edited

codesandbox-ci bot commented May 3, 2024 • edited

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsattarian May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi commented May 15, 2024

github-actions bot commented May 15, 2024 • edited

Preview in LiveCodes

mhsattarian commented May 15, 2024

dai-shi commented May 24, 2024

Choose a reason for hiding this comment

mhsattarian commented May 25, 2024

dai-shi commented May 25, 2024

mhsattarian commented May 25, 2024

dai-shi commented May 25, 2024

vercel bot commented May 3, 2024 •

edited

codesandbox-ci bot commented May 3, 2024 •

edited

mhsattarian May 22, 2024 •

edited

github-actions bot commented May 15, 2024 •

edited