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

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

Merged

Conversation

mhsattarian
Copy link
Contributor

Related Issues or Discussions

Related to #1833 (reply in thread)

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

Copy link

vercel bot commented May 3, 2024

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

Copy link

codesandbox-ci bot commented May 3, 2024

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.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still string based, right?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong:

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

Comment on lines 16 to 22
type SubscribeHandler<Value> = (
subscribe: Subscribe<Value>,
key: string,
callback: (value: Value) => void,
initialValue: Value,
) => Unsubscribe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v must be a string always.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) || '')?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return () => {
window.removeEventListener('storage', storageEventCallback)
}
if (getStringStorage()?.subscribe) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mhsattarian mhsattarian May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

window.removeEventListener('storage', storageEventCallback)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also used this in Next.js but forgot to upgrade and test the new version. this was on me, sorry.

I opened a new PR (#2585) for a possible fix (and a test for detecting this problem) but I would appreciate your opinion.

Comment on lines 201 to 204
storage.subscribe = handleSubscribe.bind(
null,
getStringStorage()!.subscribe as unknown as Subscribe<Value>,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dai-shi
Copy link
Member

dai-shi commented May 15, 2024

Hope you find time to address this.

Copy link

github-actions bot commented May 15, 2024

LiveCodes 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
Copy link
Contributor Author

@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
Copy link
Member

dai-shi commented May 24, 2024

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

Comment on lines 166 to 169
(typeof window !== 'undefined' &&
typeof window.addEventListener === 'function' &&
((key, callback) => {
if (!(getStringStorage() instanceof window.Storage)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dai-shi 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 dai-shi added this to the v2.8.2 milestone May 24, 2024
@mhsattarian
Copy link
Contributor Author

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

Are we ready for the merge?

@dai-shi
Copy link
Member

dai-shi commented May 25, 2024

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

@mhsattarian
Copy link
Contributor Author

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
Copy link
Member

dai-shi commented May 25, 2024

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.

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