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

Unable to change window.location using Object.defineProperty #5124

Closed
simon360 opened this Issue Dec 18, 2017 · 67 comments

Comments

Projects
None yet
@simon360
Copy link

commented Dec 18, 2017

Do you want to request a feature or report a bug? Report a bug

What is the current behavior?

Calling the following from inside a test suite:

Object.defineProperty(location, "hostname", {
  value: "example.com",
  writable: true
});

throws the following error:

    TypeError: Cannot redefine property: hostname
        at Function.defineProperty (<anonymous>)

What is the expected behavior?

The code should not throw an exception, and window.location.hostname === "example.com" should evaluate true.

From the looks of it, jsdom now sets window.location to be unforgeable. The only way to change the values within window.location is to use reconfigure, but (per #2460) Jest doesn't expose jsdom for tests to play around with.

Please provide your exact Jest configuration and mention your Jest, node,
yarn/npm version and operating system.

Jest version: 22.0.1
Node version: 8.6.0
Yarn version: 1.2.0
OS: macOS High Sierra 10.13.2

@simon360 simon360 changed the title Unable to change window.location using defineProperty Unable to change window.location using Object.defineProperty Dec 18, 2017

@oliverzy

This comment has been minimized.

Copy link

commented Dec 19, 2017

I have the similar issue. You can create your own JSDOMEnvironment and expose jsdom object to the global like this.

const JSDOMEnvironment = require('jest-environment-jsdom');

module.exports = class CustomizedJSDomEnvironment extends JSDOMEnvironment {
  constructor(config) {
    super(config);
    this.global.jsdom = this.dom;
  }

  teardown() {
    this.global.jsdom = null;
    return super.teardown();
  }
};

And then you can call jsdom.reconfigure in your test case as you like

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2017

That's a good workaround, thanks for sharing!

You should return super.teardown(); as it's a promise, btw

@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

Perfect, @oliverzy - I'll give that a try. Thanks!

Is there an appropriate place to document this? It seems to be a question that comes up reasonably often; hopefully, future issues could be cut down if this were integrated in the docs?

robmcguinness added a commit to Availity/sdk-js that referenced this issue Dec 19, 2017

chore(devDeps): rollback to Jest v21
facebook/jest#5124

```
Object.defineProperty(location, "hostname", {
  value: "example.com",
  writable: true
});
```

```
TypeError: Cannot redefine property: hostname
        at Function.defineProperty (<anonymous>)
```
@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

This solution didn't quite work.

Inside our test files, it seems like global is set to be JSDom's window object.

In other words, inside a test suite, global is the same as window, but inside the class that extends JSDOMEnvironment, global comes from Node's environment.

As a result, having this:

describe("test suite", () => {
  it("should not fail", () => {
    global.jsdom.reconfigure({
      url: "https://www.example.com/"
    });
  });
});

fails because global.jsdom is undefined.

I got around it by doing this, but I'm not super fussed about it.

const JSDOMEnvironment = require("jest-environment-jsdom");

module.exports = class JSDOMEnvironmentGlobal extends JSDOMEnvironment {
  constructor(config) {
    super(config);

    this.dom.window.jsdom = this.dom;
  }
};

With this environment, global.jsdom inside a test suite is equal to this.dom, and the test suite above works.

To me, it feels like setting jsdom to be a property of its own window object is bound to fall apart eventually - is there a cleaner way to do it?

@oliverzy

This comment has been minimized.

Copy link

commented Dec 19, 2017

you need to write jsdom rather than global.jsdom in your tests.

@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

@oliverzy Like this?

describe("test suite", () => {
  it("should not fail", () => {
    jsdom.reconfigure({
      url: "https://www.example.com/"
    });
  });
});

That throws jsdom is not defined, but I may be misinterpreting.

@danielbayerlein

This comment has been minimized.

Copy link

commented Dec 19, 2017

@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

@danielbayerlein my Jest config has this:

"testEnvironment": "@wel-ui/jest-environment-jsdom-global"

where @wel-ui/jest-environment-jsdom-global is the name of a package in our monorepo. The environment is getting used correctly, though, because the solution that sets jsdom on window works as expected.

@modestfake

This comment has been minimized.

Copy link

commented Dec 19, 2017

BTW, does anyone know why the original solution doesn't work in the new version?
This one:

Object.defineProperty(location, "hostname", {
  value: "example.com",
  writable: true
});
@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2017

@modestfake we have upgraded from JSDOM@9 to JSDOM@11, my guess is that they changed how the variable is defined

@modestfake

This comment has been minimized.

Copy link

commented Dec 19, 2017

@SimenB Got it. Just found a description of jsdom reconfigure method.

The top property on window is marked [Unforgeable] in the spec, meaning it is a non-configurable own property and thus cannot be overridden or shadowed by normal code running inside the jsdom, even using Object.defineProperty.

@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

I added a new repository to demonstrate this behaviour. Is anyone able to reproduce it by cloning locally?

https://github.com/simon360/test-environment-for-jest

@modestfake

This comment has been minimized.

Copy link

commented Dec 19, 2017

@simon360 reproduced
image

@modestfake

This comment has been minimized.

Copy link

commented Dec 19, 2017

@simon360 I've found. You've missed this keyword when defining global.jsdom:

const JSDOMEnvironment = require("jest-environment-jsdom");

module.exports = class JSDOMEnvironmentGlobal extends JSDOMEnvironment {
  constructor(config) {
    super(config);

    this.global.jsdom = this.dom;
  }

  teardown() {
    this.global.jsdom = null;

    return super.teardown();
  }
};
@andrewBalekha

This comment has been minimized.

@modestfake

This comment has been minimized.

Copy link

commented Dec 19, 2017

@andrewBalekha What about this?

jsdom.reconfigure({
  url: 'https://www.example.com/endpoint?queryparam1=15&queryparam2=test'
});
@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

Thanks @modestfake - sorry for the dumb mistake!

Ok, I see it now - this.global on a Jest environment object gets set as global in a Jest test file. That makes sense - thanks for helping me through it! If there's enough interest, I could package the repaired version of that repo and put it on npm as jest-environment-jsdom-global.

However, I do hope there's a cleaner way to do this in Jest in the future. This isn't a low friction way to change window.location -

Could there be a new docblock, like there is for @jest-environment? For example...

/**
 * @jest-url https://www.example.com/
 */

Or, maybe JSDom can be exposed on a special part of the jest object - something like:

jest.environment.jsdom.reconfigure({
  url: "https://www.example.com/"
});

(which would have the added benefit of being able to change window.top)

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Dec 19, 2017

We have merged #5003 now. being able to add it as a docblock might make sense, not sure. @cpojer? We could deprecate testUrl as well, as it can be provided through that new option.

If there's enough interest, I could package the repaired version of that repo and put it on npm as jest-environment-jsdom-global.

I think that makes sense in any case, as it does more than just let you set url - it exposes the full JSDOM to the environment

@msholty-fd

This comment has been minimized.

Copy link

commented Dec 19, 2017

@andrewBalekha Object.defineProperty(location, 'search', { ...options }); throws the same error as window.location. Thanks for the suggestion though.

@ekelvin

This comment has been minimized.

Copy link

commented Dec 19, 2017

Object.defineProperty(window.location, 'href', {
set: newValue => { currentUrl = newValue; },
});
I had this in previous versions and now throws error.
If I add writable: true
throws another exception that I can't specify both accessor and writable

@simon360

This comment has been minimized.

Copy link
Author

commented Dec 19, 2017

I've published a new package on npm called jest-environment-jsdom-global, which may help with the problems some people are having with Object.defineProperty.

@danielbayerlein

This comment has been minimized.

Copy link

commented Dec 21, 2017

Does anyone have a workaround for { writable: true }?

For example:

Object.defineProperty(window.location, 'href', { writable: true })

...

Object.defineProperty(window.location, 'hash', { writable: true })

...

Object.defineProperty(window.location, 'search', { writable: true })
@modestfake

This comment has been minimized.

Copy link

commented Dec 21, 2017

@danielbayerlein read this thread. You need to create custom environment. Previous message contains url with example

@danielbayerlein

This comment has been minimized.

Copy link

commented Dec 21, 2017

@modestfake I've already read this thread and #5124 (comment) works fine. But I've another use case. With Jest 21.x.x I have set Object.defineProperty(window.location, 'href', { writable: true }) without the URL - only { writable: true }. If I set the URL, then the test makes no sense.

@modestfake

This comment has been minimized.

Copy link

commented Dec 21, 2017

@danielbayerlein what the use case to make it writable but not override it actually? Maybe understanding this can help me to come up with workaround

@danielbayerlein

This comment has been minimized.

Copy link

commented Dec 21, 2017

I've a function that changes the URL.

routing.js

...

export function redirectToErrorPage () {
  window.location.href = '/error.html'
}

...

routing.test.js

test('redirect to the error page', () => {
  ...
  expect(window.location.href).toBe('/error.html')
  ...
})

With Jest 21.x.x I have set Object.defineProperty(window.location, 'href', { writable: true })

@abhijeetNmishra

This comment has been minimized.

Copy link

commented May 15, 2018

@simon360 Yes, based on my understanding of documentation, it takes about

jsdom.reconfigure({
      url: "https://www.example.com/"
    });

which overrides url globally and not per test. Please help!

@simon360

This comment has been minimized.

Copy link
Author

commented May 15, 2018

@abhijeetNmishra I'm not sure that this issue is the best place to discuss. Would you mind opening an issue on the jest-environment-jsdom-global repository where we can work through it? Thanks!

@chrislloyd

This comment has been minimized.

Copy link

commented May 23, 2018

@SimenB the stated workaround ("use jest-environment-jsdom-global") feels like an extremely suboptimal solution to what's obviously a very common problem. Anybody upgrading to Jest 22 now needs to add a dependency to that third party package and (from a user's perspective) re-write some of their tests. This is a regression in Jest's behavior.

Is there a solution to this that we could build into the default jest-environment-jsdom? Happy to make a PR with your guidance.

@ydogandjiev

This comment has been minimized.

Copy link

commented Jun 4, 2018

It's very unfortunate that someone has to jump though this many hoops just to change window.location.href. I just started using Jest and I am about to reconsider my choice of testing framework given this issue. Is there really no better solution than the ugly hacks suggested above?

@msholty-fd

This comment has been minimized.

Copy link

commented Jun 4, 2018

@ydogandjiev feel free to contribute to the project to solve this issue. Remember that this is open source, so rampaging in with comments like “unacceptable” and “ridiculous” do nothing to help anyone.

@ydogandjiev

This comment has been minimized.

Copy link

commented Jun 4, 2018

@msholty-fd I would love to help out if I can. Since I am just getting started with jest and jsdom I am not sure that I have a deep enough understanding of these libraries to know exactly what the best route for improving this experience is. Is this something best addressed in jest or jsdom? It seems like one of these libraries made a change at some point that broke the Object.defineProperty approach; was that a change made in jest or jsdom?

@ydogandjiev

This comment has been minimized.

Copy link

commented Jun 4, 2018

So here are all the options that I would consider preferable based on the number of lines required to change window.location; none of these currently work:

  1. Setting href directly doesn't work because jsdom throws an exception with message "Error: Not implemented: navigation (except hash changes)":

    window.location.href = "https://www.example.com";
    
  2. Using Object.defineProperty doesn't work because JSDOM has made the window's location property [Unforgeable]:

    Object.defineProperty(window.location, "href", {
      value: "https://www.example.com",
      configurable: true
    });
    
  3. Creating an instance of jsdom and configuring it doesn't work because jest seems to use its own instance which is not exposed for easy access:

    import { JSDOM } from "jsdom";
    ...
    const dom = new JSDOM();
    dom.reconfigure({ url: "https://www.example.com" });
    

Making options 1 or 2 work would require jsdom to backtrack on their current goals of behaving more like a real browser. Therefore, it seems like the only option we have is to make it easier to reconfigure the instance of jsdom that jest uses. Would it make sense to expose that instance directly on the global jest object; i.e. permitting something like this:

jest.dom.reconfigure({ url: "https://www.example.com" });
@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2018

I still think doing location.assign('some-url') is better than location.href = 'some-url'. I find a function call more explicit than assignment, and you can mock the function

@simon360

This comment has been minimized.

Copy link
Author

commented Jun 7, 2018

@SimenB in cases where code is trying to set location.href, yes, location.assign() is better. But if you're testing behaviour that reads location.href, location.assign() doesn't solve the problem, especially since location.assign() in JSDOM doesn't actually do anything.

The idea behind using reconfigure is to activate a code path that's only enabled when location.href is formed in a particular way. In our case, we had some code that changed depending on the current domain - the code is smelly, yes, but it's also necessary, and the best way to mitigate smelly code is to have testing fixtures that capture the behaviour and ensure that it stays in place.

@sikemausa

This comment has been minimized.

Copy link

commented Jun 28, 2018

Is there a way to use this with Enzyme and a mounted component to test for redirects?

The below test passed before upgrading Jest:

it('routes to correct route', () => {

  Object.defineProperty(window.location, 'href', {
    writable: true,
    value: 'https://mysuperawesomesite.com/',
  });

  const component = mount(
    <App {...props} />
  );

  const link = component.find('.class');

  link.simulate('click');

  expect(window.location.href).toEqual('https://mysuperawesomesite.com/new');
});

After upgrading Jest and implementing jest-environment-jsdom-global, I tried the following to no avail:

it('routes to correct route', () => {

  jsdom.reconfigure({
    url: 'https://mysuperawesomesite.com/',
  });

  const component = mount(
    <App {...props} />
  );

  const link = component.find('.class');

  link.simulate('click');

  expect(window.location.href).toEqual('https://mysuperawesomesite.com/new');
});

(window.location.href still equals 'https://mysuperawesomesite.com/', didn't get changed to ('https://mysuperawesomesite.com/new').

The click event on the element does not redirect when using this method, and the redirect occurs by setting window.location.href.

Unclear on how to properly test this or if the tests that had previously used Object.defineProperty were poorly constructed to begin with. Thanks in advance for any assistance.

EDIT: SOLVED

Was able to solve this by using window.location.assign(url) instead of window.location.href = href. This allowed me to stub out the assign method and test whether it was being properly called. See below:

it('routes to correct route', () => {
    window.location.assign = jest.fn();

    const component = mount(
      <App {...props} />
    );

    const link = component.find('.class');

    link.simulate('click');

    expect(window.location.assign).toBeCalledWith('https://mysuperawesomesite.com/new');
    window.location.assign.mockRestore();
  });
@soswow

This comment has been minimized.

Copy link

commented Jul 12, 2018

@SimenB There is a big difference between .assign and .href You can read on MDN. First one has a major cross-domain restriction. In my code I want to redirect parent page from an iframe where my code is running. Those are cross-domains. And the only way I can do this is by changing href.
I would love if this issue be re-opened, since I don't see a current workaround and I will have to just not to have test for this function. Which obviously sucks.

@trevorgk

This comment has been minimized.

Copy link

commented Jul 18, 2018

I'm in the same boat as @soswow. It would be great if you could provide a mechanism for overriding the url, as I am also removing several unit tests until this functionality is restored.

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2018

I would love if this issue be re-opened, since I don't see a current workaround and I will have to just not to have test for this function. Which obviously sucks.

There's nothing we can do on jest's side. I'm sure jsdom would love a PR supporting it. jsdom/jsdom#2112

swpease added a commit to swpease/codenames that referenced this issue Jul 21, 2018

[App] LandingPage : Game
the testing for this is, like with isRootUrlPath(), half-assed. It looks like changing the url in Jest is a bit of a pain (ref: facebook/jest#5124), so screw it, it's incredibly simple code.
@vastus

This comment has been minimized.

Copy link

commented Jul 26, 2018

Here's a simple solution that works.

describe('changing location', () => {
  const testURL = location.href;

  beforeEach(() => history.replaceState({}, 'Login', '/login'));
  afterEach(() => history.replaceState({}, 'Home', '/'));

  it('works', () => {
    expect(location.pathname).toBe('/login');
  });
});
@soswow

This comment has been minimized.

Copy link

commented Jul 26, 2018

@vastus As I explicitly pointed out - the problem is with cross-domains. history API won't allow switching to a different domain as far as I remember.

@RubenVerborgh

This comment has been minimized.

Copy link

commented Aug 23, 2018

Since location cannot be overridden directly on the jsdom window object, one possible approach is to override it on a derived object:

global.window = Object.create(window);
Object.defineProperty(window, 'location', {
  value: {
    href: 'http://example.org/'
  }
});
@sahalsaad

This comment has been minimized.

Copy link

commented Sep 3, 2018

I use this to solve the issue:

const windowLocation = JSON.stringify(window.location);
delete window.location;
Object.defineProperty(window, 'location', {
  value: JSON.parse(windowLocation)
});

Inspired by @RubenVerborgh & annemarie35

@cleverbeagle

This comment has been minimized.

Copy link

commented Oct 19, 2018

Adding an example of testing location.search based on @vastus's solution:

  test('gets passed query param and returns it as a string if it exists', () => {
    history.replaceState({}, 'Test', '/test?customer=123');
    const customerId = getQueryParam('customer');
    expect(customerId).toBe('123');
  });
@lili21

This comment has been minimized.

Copy link

commented Nov 22, 2018

@RubenVerborgh Works like a charm.

@kdelmonte

This comment has been minimized.

Copy link

commented Nov 30, 2018

try:

window.history.pushState({}, null, '/pathname?k=v');
@jackharrhy

This comment has been minimized.

Copy link

commented Dec 12, 2018

A solution similar to @sahalsaad :

const oldWindow = window.location;
delete window.location;
window.location = {
    ...oldWindow,
    // include any custom overwrites such as the following sinon stub
    replace: sinon.stub(),
};

// do your magic

window.location = oldWindow;
@alex-zaman-zocdoc

This comment has been minimized.

Copy link

commented Feb 19, 2019

@sahalsaad thanks! I used a variation of your solution to mock window.location.search:

const location = {
    ...window.location,
    search: queryString,
};
Object.defineProperty(window, 'location', {
    writable: true,
    value: location,
});
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.