fix(CookieStore): check if "localStorage" is defined before using it #8
Conversation
neilime
changed the title
src/CookieStore.ts
Outdated
| * Retrieve the global local storage if defined | ||
| * @returns Storage | undefined | ||
| */ | ||
| private getLocalStorage(): Storage | undefined { |
There was a problem hiding this comment.
I don't think we get any benefits of keeping this as a private method. We'd rather store it as a module-scoped variable, as localStorage is either defined or not (determined by the environment). Getter makes sense if the storage is eventually defined, but this is not the case.
| } | ||
| type Store = Map<string, StoreEntry> | ||
| type StoreEntry = Map<string, Cookie> | ||
| type CookieString = Omit<Cookie, 'expires'> & { expires?: string }; | ||
| type CookieString = Omit<Cookie, 'expires'> & { expires?: string } | ||
|
|
||
| export const PERSISTENCY_KEY = 'MSW_COOKIE_STORE' | ||
|
|
There was a problem hiding this comment.
I'd define the global variable here, determining if we can use local storage:
const SUPPORTS_LOCAL_STORAGE = typeof localStorage !== 'undefined'There was a problem hiding this comment.
If the localStorage is removed during runtime, it will fail. Using a getter avoid this case. What about using a method "supportsPersistency"
There was a problem hiding this comment.
What is the case when the localStorage is removed during runtime? I'd say that's an edge-case scenario that indicates an error in the runtime, but I may be wrong.
There was a problem hiding this comment.
I don't have any idea of what can be the case.
But this is technically possible.
By the way, if we store the check in a const as you recommend, I'll need extra time to find a way to test undefined localStorage. Because it cannot work as it is currently done
There was a problem hiding this comment.
@kettanaito changes done with const SUPPORTS_LOCAL_STORAGE
There was a problem hiding this comment.
Thank you for working on this, @neilime! Please, could you look into the comments I left?
It'd be great to separate the prettier changes from the actual fix, so we have a cleaner Git history. Thanks!
test/persistency.test.ts
Outdated
| store.persist() | ||
|
|
||
| localStorage = originalLocalStorage | ||
| expect(localStorage.getItem(PERSISTENCY_KEY)).toEqual(null) |
There was a problem hiding this comment.
Despite the cookies not being persisted in the local storage, I think we should still assert that they persist in the internal this.store map when you try to retrieve them.
|
🆙 😄 |
Fixes #7
localStorageis defined before using it.