Spying on ESM default export fails/inexplicably blocked

[JakobJingleheimer]

Describe the bug
Previously, a combination of import * as foo from 'someModule' + spy(foo, 'default') would work just fine (and was even listed in this repo's issues as the solution); however, now this is explicitly blocked by a manual throw.

Expected behavior
Sinon to spy on the default export.

Context (please complete the following information):

• Library version: 9.0.2

Additional context
I presume this is in order to avoid TypeError: Cannot assign to read only property

However, this can be circumnavigated by using the old skool __defineGetter__

[fatso83]

There's actually no mention of __defineGetter__ in that old issue, just Object.defineProperty, and then only to deal with transpiled Webpack modules, not actual ECMAScript Modules. Not saying it does not work, though 😄

Could you post some straight vanilla javascript example where you are able to overwrite the (non-default) exports of an imported ES Module? You need two files some-module.mjs and file-that-overwrites.mjs to test this and they need to have the .mjs extension for Node to enable the import syntax.

This test suite documents Sinon's current behavior.

[zorji]

FYI stub doesn't work for TypeScript@3.9.2+ here

[JakobJingleheimer]

@fatso83 that old issue indeed does not (but I didn't say it did 😜).

What I mean is, Sinon throws an error when detecting an ESM because later down the code Object.defineProperty(object, property, methodDesc) will fail with TypeError: Cannot assign to read only property 'default'. However, __defineGetter__ would not fail:

object.__defineGetter__(property, methodDesc.value);

Buuuut, when I tried modifying the Sinon code and running it, I got an undefined error on __defineGetter__ (perhaps a strict mode or something?).

So I'm thinking perhaps a different way, such as re-creating the object (with new values) instead of overriding its properties. More complex, but then at least do-able.

[aelbore]

FYI stub doesn't work for TypeScript@3.9.2+ here

Same issue here last working version 3.8.3

[fatso83]

@zorji and @aelbore : I have no idea why you are posting in this thread. If you are reporting a new issue, please create a separate issue and follow the normal steps for reporting (how to verify, actual code, versions, etc). We do not support Typescript and have never said so, although it should work, of course, but the details will depend on your transpilation step.

[fatso83]

So I'm thinking perhaps a different way, such as re-creating the object (with new values) instead of overriding its properties. More complex, but then at least do-able.

Is it? An ES Module is a static feature that is bound at runtime, AFAIK, and you cannot modify it.

imported features are available in the file, they are read only views of the feature that was exported. You cannot change the variable that was imported, but you can still modify properties similar to const. Additionally, these features are imported as live bindings, meaning that they can change in value even if you cannot modify the binding unlike const.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules

Looking forward to seeing if you can get anywhere with this, but I would be surprised if you (or anyone!) did.

[stale]

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.

[OmgImAlexis]

Bad bot. Can this stupid thing be turned off?

[mroderick]

@OmgImAlexis do you think there's any reason to keep this issue open?

[mroderick]

Folks, please try to keep the issue on point and only discuss what's mentioned in the original post.

It is challenging enough for the maintainers to spend our unpaid free time supporting free software, without also having to navigate orthogonal topics in the same issue.

We are delighted that Sinon (a JavaScript tool) can also be useful in the TypeScript community. However, like @fatso83 said, we do not support Typescript.

Any further off-topic comments about TypeScript in this issue will be deleted.

[OmgImAlexis]

Any further off-topic comments about TypeScript in this issue will be deleted.

@mroderick if you don't like the comments then ignore them. Actively removing comments which would help other users isn't helpful at all. Let's say you remove the comments then you'll just end up with similar users coming and commenting again and again which in turn would end up with the thread being locked.

Edit: You can also use the super easy to find "unsubscribe" button. :)

[JakobJingleheimer]

Ack sorry, I got very occupied at work. I'll try to take a look in a couple weeks. Several libraries accomplish this, so it must be possible 😁

[mroderick]

@mroderick if you don't like the comments then ignore them. Actively removing comments which would help other users isn't helpful at all. Let's say you remove the comments then you'll just end up with similar users coming and commenting again and again which in turn would end up with the thread being locked.

Edit: You can also use the super easy to find "unsubscribe" button. :)

I am a maintainer of this project, and try to read all the comments.

I can't ignore comments or unsubscribe from an issue because people decide to not play by the rules set out, ignore the guidance offered and hijack the original issue for their own purposes. It is my responsibility to try to resolve issues in a polite, tactful and efficient manner. Hijacking issues makes it harder than it needs to be to support the community.

Please help me help you and the rest of the community.

[stale]

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.

[OmgImAlexis]

Bad bot.

[mroderick]

@jshado1 do you think this issue should stay open?

[fatso83]

I see that @jshado1 says this used to work fine, but I don't think this has ever worked in vanilla un-transpiled javascript. Because it should not. It can work when transpiling modules using Babel/Webpack or similar, because you only emulate modules and not actually create a read only object in the runtime (which modules are). The throw clause was added by me, because it would otherwise throw a little bit later when you tried changing the unchangeable. Runtimes/module loaders such as esm change the field a bit, though.

If you think the behavior is wrong, I suggest just editing node_modules/sinon/lib/sinon/spy.js, remove the throws clause and report back. No hard feelings. Promise 🦄 🌈

[fatso83]

To quote @RyanCavanaugh (microsoft/TypeScript#38568 (comment))

If you're targeting commonjs but writing ES module imports/exports, TS is still going to try to give you ES module behavior.

This code is effectively not legal ES, since it's attempting to modify (indirectly via sinon.stub) a readonly property (the properties of something imported with import * as name are not mutable). It should never have worked.

Which is basically what I have been saying. So closing, as the results of this is entirely dependent on you using a transpilation step, how the transpiler does that step, etc. If it tries to create a result that is somewhat in accordance with the ESM spec, it should not work as the exports of the ES Modules are immutable (not the actual objects bound by those exports, of course).

[JakobJingleheimer]

@mroderick @fatso83 sorry i never got that time off and have been slammed at work since. It's been several months, but if memory serves, I had a very rudimentary (and dirty AF) working example back then to confirm it did work (but indeed probably shouldn't have). BUT, I think that's not the way to go beccccauuse: I believe there is / will be an official way to do this via ems loaders: https://github.com/nodejs/node/blob/master/doc/api/esm.md#loaders

These are currently unstable (explicitly stated to soon be changed), so probably hold off til they get more stable. But once they do, I think that's the way to go.

[JakobJingleheimer]

Update: This is currently being discussed in nodejs/node#36396.

I think my proposal in this comment would make this very simple. A user could manually supply a map, and the esm loader could use that to substitute:

// const mocksMap = { 'serviceA.js': 'serviceA.mock.js' };

const mock = mocksMap[importPath]; // note: it's not called `importPath` in the loader hooks
if (mock) // …

and/or, the esm loader could look for a mock match based on filename (quick n dirty example):

const ext = path.ext(importPath);
const filename = path.basename(importPath, ext);
const mockFile = await import(`${filename}.mock${ext}`);

if (mockFile) // …

[fatso83]

@jshado1 Thanks for providing that link. The linked issue is essentially about trying to find a standardized way of handling the issue of substituing dependencies on the link level, which is similar to what tools such as rewire and proxyquire does (which we describe in this how-to on our home page). In any case, this is not something Sinon should or will solve, as it is out of scope and how to solve it will be entirely dependant on your environment.