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

Accessing localStorage throws error in CookieStore #9

Closed
weyert opened this issue Dec 5, 2021 · 5 comments · Fixed by #10
Closed

Accessing localStorage throws error in CookieStore #9

weyert opened this issue Dec 5, 2021 · 5 comments · Fixed by #10

Comments

@weyert
Copy link
Contributor

weyert commented Dec 5, 2021

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @mswjs/cookies@0.1.6 for the project I'm working on.

The problem is that when using jsdom together with node-fetch in Next.js can in some occasions throw an error because it tries to access origin. I am getting the following error thrown Cannot read properties of null (reading '_origin'):

    FetchError: request to http://localhost:54391/ failed, reason: Cannot read properties of null (reading '_origin')

      at ClientRequestOverride.<anonymous> (node_modules/next/node_modules/node-fetch/lib/index.js:1461:11)
      at ClientRequestOverride.<anonymous> (node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/createClientRequestOverride.ts:309:14)
      at ClientRequestOverride.<anonymous> (node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/createClientRequestOverride.ts:196:14)
      at step (node_modules/@mswjs/interceptors/lib/interceptors/ClientRequest/createClientRequestOverride.js:33:23)
      at Object.next (node_modules/@mswjs/interceptors/lib/interceptors/ClientRequest/createClientRequestOverride.js:14:53)
      at fulfilled (node_modules/@mswjs/interceptors/lib/interceptors/ClientRequest/createClientRequestOverride.js:5:58)

Looks like it's related to trying to access localStorage when running in the jsdom environment; it expects the document.origin to exist which isn't the case when running server-side, the following line is problematic:
https://github.com/jsdom/jsdom/blob/e46f76f7e311447213a3a3be1526db3d53028ee5/lib/jsdom/browser/Window.js#L417-L425

Here is the diff that solved my problem:

diff --git a/node_modules/@mswjs/cookies/lib/CookieStore.js b/node_modules/@mswjs/cookies/lib/CookieStore.js
index 567800c..a4671f3 100644
--- a/node_modules/@mswjs/cookies/lib/CookieStore.js
+++ b/node_modules/@mswjs/cookies/lib/CookieStore.js
@@ -15,6 +15,21 @@ exports.PERSISTENCY_KEY = void 0;
 const set_cookie_parser_1 = require("set-cookie-parser");
 exports.PERSISTENCY_KEY = 'MSW_COOKIE_STORE';
 const SUPPORTS_LOCAL_STORAGE = typeof localStorage !== 'undefined';
+
+function isLocalStorageSupported() {
+    try {
+        if (typeof localStorage === 'undefined') {
+            return false
+        }
+
+        localStorage.setItem('test', 'test')
+        const item = localStorage.getItem('test', 'test')
+        return true
+    } catch (err) {
+        return false
+    }
+}
+
 class CookieStore {
     constructor() {
         this.store = new Map();
@@ -89,9 +104,10 @@ class CookieStore {
      * Hydrates the virtual cookie store from the `localStorage` if defined.
      */
     hydrate() {
-        if (!SUPPORTS_LOCAL_STORAGE) {
+        if (!isLocalStorageSupported()) {
             return;
         }
+
         const persistedCookies = localStorage.getItem(exports.PERSISTENCY_KEY);
         if (persistedCookies) {
             try {
@@ -128,7 +144,7 @@ Invalid value has been removed from localStorage to prevent subsequent failed pa
      * so they are available on the next page load.
      */
     persist() {
-        if (!SUPPORTS_LOCAL_STORAGE) {
+        if (!isLocalStorageSupported()) {
             return;
         }
         const serializedCookies = Array.from(this.store.entries()).map(([origin, cookies]) => {

This issue body was partially generated by patch-package.

weyert pushed a commit to weyert/cookies that referenced this issue Dec 5, 2021
…Node

Internally in `jsdom` has a getter function defined for `localStorage`
that can in some circumstances throw a runtime error that is not handled
by the package

The code has changed to use a function to qeury whether `localStorage` is
available in the runtime context. If accessing `localStorage` global throws
an error it returns an error otherwise `true`

fixes mswjs#9
@kettanaito
Copy link
Member

Hey, @weyert. Thanks for reporting this.

Could you please share a reproduction repository?

The error you get is caused by jsdom being corrupted. Evaluating the next line fails in jsdom:

idlUtils.implForWrapper(this._document)._origin === "null"

Which causes the said exception. That's not something this library or you should be doing directly, so before we address this we need to take a closer look at it.

While this is rather an edge case, there are scenarios when accessing localStorage is forbidden despite it being present. For example, accessing the storage in a private browser window throws a runtime exception. We should consider taking that into account, I suppose.

@sAs59
Copy link

sAs59 commented Jan 19, 2022

The same error happens in incognito when third-party cookies are blocked and used inside iframe with different origin. Do you consider merging PR?

@kettanaito
Copy link
Member

@sAs59, localStorage is forbidden in incognito, afaik. Pull requests are always welcome, though, if you think there's a way to address this, but in this case is the browser limitation.

@sAs59
Copy link

sAs59 commented Jan 19, 2022

PR provided by @weyert already addresses the issue. Please merge it if possible. E2E test cases can also be a good example of different use case, which run in incognito

kettanaito pushed a commit to weyert/cookies that referenced this issue Jan 19, 2022
…Node

Internally in `jsdom` has a getter function defined for `localStorage`
that can in some circumstances throw a runtime error that is not handled
by the package

The code has changed to use a function to qeury whether `localStorage` is
available in the runtime context. If accessing `localStorage` global throws
an error it returns an error otherwise `true`

fixes mswjs#9
kettanaito added a commit that referenced this issue Jan 19, 2022
* fix: cover the case were accessing `localStorage` can throw error in Node

Internally in `jsdom` has a getter function defined for `localStorage`
that can in some circumstances throw a runtime error that is not handled
by the package

The code has changed to use a function to qeury whether `localStorage` is
available in the runtime context. If accessing `localStorage` global throws
an error it returns an error otherwise `true`

fixes #9

* fix(localStorage): merge tests when localStorage is undefined

* chore(CookieStore): change function name to "supportsLocalStorage"

Co-authored-by: Weyert de Boer <weyert.deboer@tapico.io>
Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com>
@github-actions
Copy link

🎉 This issue has been resolved in version 0.1.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants