TypeError thrown when attempting to td.replace a getter property

[gurpreetatwal]

Description

I attempted to use td.replace(object, 'property') to replace a property that is defined as a getter using Object.defineProperty and got an error.

Issue

Attempting to set a getter-only property leads to a TypeError being thrown by Node.js in contexts where strict mode is active. This MDN article has some more details about this error condition.

I did some debugging and was able to trace down the issue to the following code in src/replace/property.js (specifically L15) https://github.com/testdouble/testdouble.js/blob/360c259cad3f8956e2bfd4d45b7b8dfd8bad2f74/src/replace/property.js#L14-L16

Environment

• node -v: v14.8.0
• npm -v: 6.14.7
• npm ls testdouble: [email protected]

Failing Test

https://github.com/gurpreetatwal/testdouble.js/commit/c16e8325b509c0eddb4f0bd24bb6b4c470d6fa57

Runkit Notebook

https://runkit.com/gurpreetatwal/testdouble-getter-issue

Code-fenced Examples

var td = require('testdouble');

const dependency = {};

Object.defineProperty(dependency, 'foo', {
  enumerable: true,
  get() {
    return 'bar';
  },
});

td.replace(dependency, 'foo'); // TypeError: Cannot set property foo of #<Object> which has only a getter

Note: This error might become more common as it seems like Typescript v3.9 uses defineProperty for export statements. I encountered because my unit tests broke while I was in the process of upgrading the @sentry/node package from 5.21.0 to 5.21.1. I noticed that they had bumped the version of Typescript that they were using to v3.9 ( getsentry/sentry-javascript#2811) in the v5.21.1 release. I took a look at the difference in the builds of v5.21.0 and v5.21.1 and noticed the sudden appearance of defineProperty.

node_modules/@sentry/node/dist/index.js

-exports.captureException = core_1.captureException;
+Object.defineProperty(exports, "captureException", { enumerable: true, get: function () { return core_1.captureException; } });

[searls]

Great issue report and thanks for committing a failing test while you're at it (link to referenced commit: https://github.com/gurpreetatwal/testdouble.js/commit/c16e8325b509c0eddb4f0bd24bb6b4c470d6fa57 )

In the meantime, I assume you were doing a td.replace('sentry') somewhere? And it was by automatically imitating something in sentry that broke it when you upgraded? Using replace on third party dependencies is possible but not proactively supported by td.js.

Anyway, this is an edge case but one we probably want to be thoughtful about. Curious what @lgandecki thinks about this one.

[gurpreetatwal]

Thank you! I appreciate the work it takes to maintain a project and triage issues for a project so try to makes easier whenever I can haha.

It wasn't exactly what you mentioned, we try to stay away from the style of td.replace('module') for the reasons you mentioned. I was doing property replacement by doing td.replace(sentry, 'captureException') where sentry is essentially an instance of the SDK (the whole sentry SDK is actually a singleton which makes things weirder, but that's not relevant for here).

Definitely agree that this is an edge case and probably affects more than just td.replace

[searls]

Ah, well if that was your actual usage then I'd really like for that to actually work (even though the "right" answer might be to have a wrapper you own that delegates to sentry for that function, I get why you're probably doing it)

[lgandecki]

Great debugging. I'm not sure what to do about this right away. I looked into this a bit and seems that, luckily, not all exports are getting transpiled in this way, only the

export {something} from 'something' 

are. Not very reassuring for this particular case, I know. Also, it's a common pattern for libraries to have an index.js with exports like that, so this definitely won't be a one-off edge-case.

If I find some time and inspiration I might try to fix this. If we at TheBrain/Xolv.io encounter the same problem somewhere (and we use TypeScript everywhere nowadays) then the priority will definitely get higher. ;)

I second what Justin is suggesting - you could make a wrapper, it would make the testing much cleaner. I realize though it's not a change you'd like to do because of a patch update of a 3rd party dependency.

[giltayar]

Stale. Closing. Please reopen if still relevant and I will look into it.