fix(utils): subscribe method of createJSONStorage should only run in the client #2585
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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. |
|
| Playground | Link |
|---|---|
| React demo | https://livecodes.io?x=id/FHKC76S5Z |
See documentations for usage instructions.
|
@dai-shi Hi again. this is the simplest fix I could think of. is it enough in your opinion? should we add a try/catch block to ensure subscribe method won't fail? |
…not be called in server
|
Unfortunately, it's not what I expect. Let me try and see how it goes. |
|
oh, this is tricky. what I expect doesn't work. |
|
How does 6ca781f look? I wanted to keep this: https://github.com/pmndrs/jotai/pull/2539/files#diff-4bb714dba559e7b7df1c7cb13113df5aefbafe023dac97e346bb39e8db159a1aL150-L152 |
|
Awesome. this looks better. |
|
I should remove the test I added to check the subscriber not be called on server. |
…rage to not be called in server" This reverts commit 71d2fa0.
|
but now can we make sure such a case won't happen in the future? |
Ideally, there should be a test that throw on |
|
can't we patch |
|
can we? Please give it a try! |
…JSONStorage in server
…ess with createJSONStorage in server
|
honestly, I couldn't wrap my head around this problem as much as I wanted. since we are already calling the last commit seems eligible. what do you think @dai-shi? |
|
I feel like I'm commit-polluting (if such a word exists) but in my defense, I should say these TS errors were not thrown in my local tests. anyways, sorry. :) |
|
How about this? Object.defineProperty(window, 'localStorage', {
get() {
throw new Error('localStorage in not available.')
},
})It's probably different from Node.js case, but if we removed try-catch, the test would fail. |
|
same behavior with less code. I'm in. |
| Object.defineProperty(window, 'localStorage', { | ||
| get() { | ||
| return localStorage | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This doesn't really restore, but maybe it's fine.
There was a problem hiding this comment.
you're right. I removed this part of the code and nothing changed. doesn't it affect using the localStorage in other tests?
There was a problem hiding this comment.
It may. the best is to leave it for now.
| expect(storage.subscribe).toBeUndefined() | ||
| }) | ||
|
|
||
| it('createJSONStorage with localStorage', async () => { | ||
| expect(() => createJSONStorage(() => window.localStorage)).not.toThrow() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I tested with the try/catch removed and it did throw an error:
should we add a new test or modify this one in you opinion?
There was a problem hiding this comment.
thanks. that's my oversight. yeah, the current test seems good. no need to add new one.
There was a problem hiding this comment.
oh, I think what we need to test, if possible, is createJSONStorage(). Do you have any idea?
There was a problem hiding this comment.
you mean instead of createJSONStorage(() => window.localStorage) we should test createJSONStorage()?
There was a problem hiding this comment.
Yes, if possible, we want to test both. Otherwise, it's fine for now.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It fails if we remove the fix, right?
Yes
Also, I added console.warn patching in the last commit, please check it.
|
I'm glad I could be of any help. 🙏 |
dai-shi
changed the title
…the client (pmndrs#2585) * fix: prevent calling storage subscribe unless in client * test: add test for getStringStorage method from createJSONStorage to not be called in server * use try-catch * Revert "test: add test for getStringStorage method from createJSONStorage to not be called in server" This reverts commit 71d2fa0. * test: checking localStorage/sessionStorage access with createJSONStorage in server * test: improve checking localStorage/sessionStorage access with createJSONStorage in server * test: use defineProperty for checking localStorage/sessionStorage access with createJSONStorage in server * fix: ts error in new test * improve detecting browser storage access in client * Update src/vanilla/utils/atomWithStorage.ts * patch console.warn for atomWithStorage (in non-browser environment) test --------- Co-authored-by: daishi <daishi@axlight.com> Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Related Issues or Discussions
Fixes #2581
Summary
Fixes some issues created by previous PR (#2539) subscribe method of createJSONStorage should only run in the client
Check List
pnpm run prettierfor formatting code and docs