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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 60 additions & 21 deletions src/vanilla/utils/atomWithStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,21 @@ import { RESET } from './constants.ts'
const isPromiseLike = (x: unknown): x is PromiseLike<unknown> =>
typeof (x as any)?.then === 'function'

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

type Unsubscribe = () => void

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.

type SetStateActionWithReset<Value> =
| Value
| typeof RESET
Expand Down Expand Up @@ -38,12 +51,14 @@ export interface AsyncStringStorage {
getItem: (key: string) => PromiseLike<string | null>
setItem: (key: string, newValue: string) => PromiseLike<void>
removeItem: (key: string) => PromiseLike<void>
subscribe?: Subscribe<string | null>
}

export interface SyncStringStorage {
getItem: (key: string) => string | null
setItem: (key: string, newValue: string) => void
removeItem: (key: string) => void
subscribe?: Subscribe<string | null>
}

export function withStorageValidator<Value>(
Expand Down Expand Up @@ -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?

if (!(getStringStorage() instanceof window.Storage)) {
return () => {}
}
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 || '')

}
}
window.addEventListener('storage', storageEventCallback)
return () => {
window.removeEventListener('storage', storageEventCallback)
}
}

const handleSubscribe: SubscribeHandler<Value> = (
subscriber,
key,
callback,
initialValue,
) => {
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.

} catch {
newValue = initialValue
}

callback(newValue as Value)
}

return subscriber(key, callbackWithParser, initialValue)
}

const storage: AsyncStorage<Value> | SyncStorage<Value> = {
getItem: (key, initialValue) => {
const parse = (str: string | null) => {
Expand Down Expand Up @@ -141,30 +192,18 @@ export function createJSONStorage<Value>(
),
removeItem: (key) => getStringStorage()?.removeItem(key),
}

if (
typeof window !== 'undefined' &&
typeof window.addEventListener === 'function' &&
window.Storage
typeof window.addEventListener === 'function'
) {
storage.subscribe = (key, callback, initialValue) => {
if (!(getStringStorage() instanceof window.Storage)) {
return () => {}
}
const storageEventCallback = (e: StorageEvent) => {
if (e.storageArea === getStringStorage() && e.key === key) {
let newValue: Value
try {
newValue = JSON.parse(e.newValue || '')
} catch {
newValue = initialValue
}
callback(newValue)
}
}
window.addEventListener('storage', storageEventCallback)
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.

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.

} else if (window.Storage) {
storage.subscribe = handleSubscribe.bind(null, webStorageSubscribe)
}
}
return storage
Expand Down
99 changes: 99 additions & 0 deletions tests/react/vanilla-utils/atomWithStorage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
createJSONStorage,
unstable_withStorageValidator as withStorageValidator,
} from 'jotai/vanilla/utils'
import type { SyncStringStorage } from 'jotai/vanilla/utils/atomWithStorage'

const resolve: (() => void)[] = []

Expand Down Expand Up @@ -601,3 +602,101 @@ describe('withStorageValidator', () => {
atomWithStorage('my-number', 0, withStorageValidator(isNumber)(storage))
})
})

describe('with subscribe method in string storage', () => {
it('createJSONStorage subscriber is called correctly', async () => {
const store = createStore()

const subscribe = vi.fn()
const stringStorage = {
getItem: () => {
return null
},
setItem: () => {},
removeItem: () => {},
subscribe,
}

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

const dummyAtom = atomWithStorage<number>('dummy', 1, dummyStorage)

const DummyComponent = () => {
const [value] = useAtom(dummyAtom, { store })
return (
<>
<div>{value}</div>
</>
)
}

render(
<StrictMode>
<DummyComponent />
</StrictMode>,
)

expect(subscribe).toHaveBeenCalledWith('dummy', expect.any(Function), 1)
})

it('createJSONStorage subscriber responds to events correctly', async () => {
const storageData: Record<string, string> = {
count: '10',
}

const stringStorage = {
getItem: (key: string) => {
return storageData[key] || null
},
setItem: (key: string, newValue: string) => {
storageData[key] = newValue
},
removeItem: (key: string) => {
delete storageData[key]
},
subscribe(key, callback) {
function handler(event: CustomEvent<string>) {
callback(event.detail)
}

window.addEventListener('dummystoragechange', handler as EventListener)
return () =>
window.removeEventListener(
'dummystoragechange',
handler as EventListener,
)
},
} as SyncStringStorage

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

const countAtom = atomWithStorage('count', 1, dummyStorage)

const Counter = () => {
const [count] = useAtom(countAtom)
return (
<>
<div>count: {count}</div>
</>
)
}

const { findByText } = render(
<StrictMode>
<Counter />
</StrictMode>,
)

await findByText('count: 10')

storageData.count = '12'
fireEvent(
window,
new CustomEvent('dummystoragechange', {
detail: '12',
}),
)
await findByText('count: 12')
// expect(storageData.count).toBe('11')
})
})