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

localStorage implementation can't be spied on. webidl converter issue? #2318

Closed
vlad0 opened this Issue Aug 3, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@vlad0
Copy link

vlad0 commented Aug 3, 2018

Basic info:

  • Node.js version: v9.11.2
  • jsdom version: 11.12.0

Minimal reproduction case

In the latest release 11.12.0 jsdom implemented localStorage and sessionStorage support. That's really awesome and great but the issue I am experiencing is that we can't spy On it or mock it during testing with jest.

I've created a simple repo for the use case at: https://github.com/vlad0/jest-localstorage-issue

As much as I hate to do this but I'd like to give a link to the jest issue I've created facebook/jest#6798

and mainly because of the setter implementation(webidl I guess):

What I have discovered is that in ./node_modules/jsdom/lib/jsdom/living/generated/Storage.js the issue is with the set method of the Proxy

Currently the code below is on line 278.
if (ownDesc === undefined) {
          const parent = Reflect.getPrototypeOf(target);
          if (parent !== null) {
            return Reflect.set(parent, P, V, receiver);
          }
          ownDesc = { writable: true, enumerable: true, configurable: true, value: undefined };
        }
if we remove receiver from return Reflect.set(parent, P, V, receiver); we will be able to spy on it. But I guess that's coming from webidl converter

How does similar code behave in browsers?

(Link to a jsbin or similar suggested.)

@Zirro

This comment has been minimized.

Copy link
Member

Zirro commented Aug 3, 2018

Thanks for the report. Unfortunately I don't think any of us are very familiar with the methods offered by Jest, since we're a separate project.

Despite @SimenB's reply - if a long-term fix is needed, it seems like the proper location for it is in Jest.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 3, 2018

In general, the question to ask is, how would you spy on these methods in the browser? That's how you need to do it in jsdom. We aim to provide what the browser provides---no less, and no more.

@Zirro

This comment has been minimized.

Copy link
Member

Zirro commented Aug 5, 2018

Closing this given the conclusion above.

@Zirro Zirro closed this Aug 5, 2018

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Aug 5, 2018

You could just wrap the Proxy with another Proxy. It's definitely nontrivial to implement, but that's how it is in browsers too.

@SimenB

This comment has been minimized.

Copy link
Contributor

SimenB commented Aug 14, 2018

JSDOM behaves differently from browsers here (at least Chrome).

See the following snippet:

console.log("before", localStorage.setItem);
localStorage.setItem = (...args) => console.log(...args);
console.log("after", localStorage.setItem);

Running this in Chrome (v68) prints the following:

before ƒ setItem() { [native code] }
after (...args) => console.log(...args)

This is the same code running in jsdom:

const jsdom = require("jsdom");
const { JSDOM } = jsdom;

const { window } = new JSDOM(``, {
  runScripts: "outside-only",
  url: "https://example.org/",
});

window.eval(`
console.log("before", localStorage.setItem);
localStorage.setItem = (...args) => console.log(...args);
console.log("after", localStorage.setItem);
`);

Running it gives the following:

"before"
function setItem()
"after"
function setItem()

Runnable example: https://runkit.com/embed/xa6sc3gpafch

As you can see, replacing the function works in Chrome, but not in JSDOM.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 14, 2018

Thanks. Unfortunately I guess this is an area where Chrome is not compatible with the Web IDL standard; if you test the same code in Firefox, for example, it will not work. I will file a Chrome bug to get them to fix it.

Edit: found the bug. https://bugs.chromium.org/p/chromium/issues/detail?id=510994

@SimenB

This comment has been minimized.

Copy link
Contributor

SimenB commented Aug 14, 2018

Safari behaves the same as Chrome, FWIW. However, Edge matches Firefox.

Is it possible at all to replace localStorage or any of its functions? Assigning to, or using Object.defineProperty on, window.localStorage does nothing

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 14, 2018

You can replace localStorage itself, in the usual way you replace a getter (by using Object.defineProperty).

@SimenB

This comment has been minimized.

Copy link
Contributor

SimenB commented Aug 14, 2018

That works in Chrome, but not JSDOM.

console.log("before", localStorage.setItem);
Object.defineProperty(window, 'localStorage', {value: {}})
console.log("after", localStorage.setItem);

Chrome:

before ƒ setItem() { [native code] }
after undefined

JSDOM:

"before"
function setItem()
"after"
function setItem()
undefined

EDIT: It also doesn't work in Firefox (it behaves the same as JSDOM), so at least it's consistent there

@fernandopasik

This comment has been minimized.

Copy link

fernandopasik commented Aug 20, 2018

The ideal situation would be to being able to spy the methods, but it seems we can't replace the whole localStorage either

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 20, 2018

@SimenB your example works fine in jsdom from what I can see: https://runkit.com/embed/tyjajqz5j8hu

Also works fine in my Firefox console.

@SimenB

This comment has been minimized.

Copy link
Contributor

SimenB commented Sep 16, 2018

@domenic sorry about the slow response! You can modify it from the outside, but not from the inside.

That is, your example works (modifying the returned window from jsdom), but not this one (modifying the window from an eval-ed script): https://runkit.com/simenb/5b9ea048f459f700125e9fbc

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 18, 2018

Ah, I see! That's a Node.js bug then. @TimothyGu was tracking it down last I heard... curious what the latest is.

@paradite

This comment has been minimized.

Copy link

paradite commented Sep 20, 2018

Hi, can we re-open this issue, or create a new issue for window.localStorage cannot be mocked or substituted?

I have tried the following but all don't work as expected (cannot substitute getItem method):

global.localStorage = {
  getItem: () => 'false',
  setItem: () => {},
  TEST: 'test',
};

which gives

console.log(window.localStorage);
// -> Storage {}
window.localStorage.setItem('KEY', 'true');
console.log(window.localStorage.getItem('KEY'));
// -> 'true'

And

Object.defineProperty(window, 'localStorage', {
  value: {
    getItem: () => 'false',
    setItem: () => {},
    TEST: 'test',
  },
});

which produces

console.log(window.localStorage);
// -> Storage {}
window.localStorage.setItem('KEY', 'true');
console.log(window.localStorage.getItem('KEY'));
// -> 'true'

And

global.localStorage.TEST = 'test';
global.localStorage.getItem = () => 'false';

which gives

console.log(window.localStorage);
// -> Storage { TEST: 'test' }
window.localStorage.setItem('KEY', 'true');
console.log(window.localStorage.getItem('KEY'));
// -> 'true'
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 20, 2018

No, since this is a Node.js issue outside our control, the best place to have an open issue would be on their issue tracker, not here.

@paradite

This comment has been minimized.

Copy link

paradite commented Sep 20, 2018

@domenic Hi, thanks for the reply.
I am having trouble understanding why is this a Node.js issue?
In previous versions of jsdom (9.12.0 to be specific), the mocking worked.
I am under the impression that the addition of localStorage to jsdom (3afbc0f) caused this issue of localStorage not being able to be mocked anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.