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

fix(utils): subscribe method of createJSONStorage should only run in the client #2585

Merged
merged 12 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
13 changes: 10 additions & 3 deletions src/vanilla/utils/atomWithStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,22 @@ export function createJSONStorage<Value>(
callback(newValue)
})

let subscriber = getStringStorage()?.subscribe
let subscriber: StringSubscribe | undefined
try {
subscriber = getStringStorage()?.subscribe
} catch {
// ignore
}
if (
!subscriber &&
typeof window !== 'undefined' &&
typeof window.addEventListener === 'function' &&
window.Storage &&
getStringStorage() instanceof window.Storage
window.Storage
) {
subscriber = (key, callback) => {
if (!(getStringStorage() instanceof window.Storage)) {
return () => {}
}
const storageEventCallback = (e: StorageEvent) => {
if (e.storageArea === getStringStorage() && e.key === key) {
callback(e.newValue)
Expand Down
44 changes: 43 additions & 1 deletion tests/react/vanilla-utils/atomWithStorage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -354,20 +354,62 @@ describe('atomWithStorage (in non-browser environment)', () => {
}

const addEventListener = window.addEventListener
const localStorage = window.localStorage
const sessionStorage = window.sessionStorage
const consoleWarn = window.console.warn

beforeAll(() => {
;(window as any).addEventListener = undefined
// patch console.warn to prevent logging along test results
Object.defineProperty(window.console, 'warn', {
value: () => {},
})
Object.defineProperties(window, {
localStorage: {
get() {
throw new Error('localStorage is not available.')
},
},
sessionStorage: {
get() {
throw new Error('sessionStorage is not available.')
},
},
})
})

afterAll(() => {
window.addEventListener = addEventListener
Object.defineProperty(window.console, 'warn', {
value: consoleWarn,
})
Object.defineProperties(window, {
localStorage: {
get() {
return localStorage
},
},
sessionStorage: {
get() {
return sessionStorage
},
},
})
})

it('createJSONStorage with undefined window.addEventListener', async () => {
const storage = createJSONStorage(() => asyncDummyStorage)

expect(storage.subscribe).toBeUndefined()
})

it('createJSONStorage with localStorage', async () => {
expect(() => createJSONStorage()).not.toThrow()
expect(() => createJSONStorage(() => window.localStorage)).not.toThrow()
Copy link
Member

Choose a reason for hiding this comment

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

What we should check is not only createJSONStorage, but also atomWithStorage.
You want to confirm if the test works as expected by removing the try-catch we added.

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 tested with the try/catch removed and it did throw an error:

CleanShot 2024-05-30 at 15 17 44@2x

should we add a new test or modify this one in you opinion?

Copy link
Member

Choose a reason for hiding this comment

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

thanks. that's my oversight. yeah, the current test seems good. no need to add new one.

Copy link
Member

Choose a reason for hiding this comment

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

oh, I think what we need to test, if possible, is createJSONStorage(). Do you have any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean instead of createJSONStorage(() => window.localStorage) we should test createJSONStorage()?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if possible, we want to test both. Otherwise, it's fine for now.

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 simply added another expect to check for it, but I have a feeling you mean something else.
there is also a console.warn happening here due to the checking we have in createJSONStorage. is it ok?

here is a screenshot:

CleanShot 2024-06-02 at 13 06 43@2x

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

except(() => createJSONStorage()).not.toThrow() seems good. (It fails if we remove the fix, right?)

As for the console.warn, let's mock it for this test and hide the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fails if we remove the fix, right?

Yes

Also, I added console.warn patching in the last commit, please check it.

})

it('createJSONStorage with sessionStorage', async () => {
expect(() => createJSONStorage(() => window.sessionStorage)).not.toThrow()
})
})

describe('atomWithStorage (with browser storage)', () => {
Expand Down
Loading