Sinon: Attempting to configurable attribute of unconfigurable property.

[gyandeeps]

System info

Node: 8
babel-polyfill: 6.23.0
core-js: ^2.4.0
phantomjs-prebuilt: 2.1.14
Sinon: 2.3.1 (no issue) - 2.3.2 (has issue)

What did you expect to happen?

• It runs fine on chrome
• It throws error on phantomjs
• No issues with sinon 2.3.1. Issue after that.

What actually happens

I use karma run unit test. webpack for bundling and babel for code translation and polyfill. Running my unit test using phantomjs and i get the following error:

PhantomJS 2.1.1 (Windows 7 0.0.0) Positioning detectFlipPositionY Special cases Should return original if element exceeds limit on top and bottom FAILED
TypeError: Attempting to configurable attribute of unconfigurable property. in C:/Users/<dir/dir>/node_modules/babel-polyfill/dist/polyfill.js (line 4908)

How to reproduce

describe("Positioning", () => {
    const sandbox = sinon.sandbox.create();

    const mockWindowWithAttr = (width = 100, height = 100) => {
        sandbox.stub(window, "innerWidth").value(width);
        sandbox.stub(window, "innerHeight").value(height);
    };

    beforeEach(() => {
        mockWindowWithAttr();
    });

    afterEach(() => {
        sandbox.restore();
    });

    it("test", () => {
        assert.isTrue(true);
    });
});

UPDATE

Regression: https://github.com/sinonjs/sinon/pull/1419

• If I remove configurable: true then it works fine.

[fatso83]

Where do you mean you set the configurable option? It's not in the code. Thanks for finding the regression btw

[gyandeeps]

1419 PR sets a configurable: true at many places when using Object.defineProperty. Thats where If i remove that then it works fine..

[fatso83]

👌. I guess we could wrap those defineProperty calls in a try/catch. Unless there is some way of knowing if they are configurable before attempting?

[lucasfcosta]

@fatso83 Yup, there's an easy way to do it.

We can use Object.getOwnPropertyDescriptor while going all the way up the prototype chain to find this properties' property descriptor (because that method only returns own property descriptors). Btw, we actually have a utility method that already does this.

Then we need to check if that property exists and if it's configurable in order to decide how Object.defineProperty will behave.

This change should be enough to fix this issue.

@gyandeeps if you want to do a PR for this I'd be more than happy to review it. I don't think I will have enough time to work on this in the next two weeks, so if anyone wants to tackle it I highly encourage you to do so. Otherwise I can work on it (alongside the other ongoing things I have got here) when I come back.

[fatso83]

@lucasfcosta There is a PR ready for review from you at #1462 :-)

[mroderick]

1462 didn't fix this in all environments, the build fails in Safari 9.0

1) issues #1456 stub window innerHeight:
Attempting to change access mechanism for an unconfigurable property.

Unfortunately, I was a bit too fast at getting a new release out the door, so now we have v2.3.5 out there. I am trying to figure out if I can unpublish that version.

[mroderick]

I am trying to figure out if I can unpublish that version.

That doesn't work currently. I think we should just leave it as is, and then fix that error asap.

[fatso83]

Maybe it's OK to just drop the failing test? There isn't a way of "fixing" the test for Safari anyhow - it doesn't allow modifying that property.

[mroderick]

I liked your proposal about using try/catch. Could that be applicable here?

[lucasfcosta]

We cannot unpublish but we can move the latest tag to a previous version, which means people would not be installing the one with bugs when using npm install sinon (or yarn add sinon if you're one of the cool kids 😆).

From the NPM docs for dist-tag:

Publishing a package sets the latest tag to the published version unless the --tag option is used. For example, npm publish --tag=beta.
By default, npm install (without any @ or @ specifier) installs the latest tag.

This is the command you've gotta use for that:

npm dist-tag add <pkg>@<old_version_number> latest

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.