feat: further es module detection

[FauxFaux]

Typescript 3.9 and above generate a different type of ES Module,
which we don't currently provide an error for; the mocks just
silently fail. Detect this type of module, and explicitly throw
an explanation here, instead of failing silently.

Purpose (TL;DR)

Throw an error for new types of ES Modules. We already try and do this, but it doesn't pick up typescript's ES modules.

Background (Problem in detail)

Here's the typescript team breaking us. microsoft/TypeScript#38568

How to verify - mandatory

1. Check out this branch
2. npm install
3. Run tests against a broken project, and get a nice error.

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[fatso83]

Hi, thanks for this. Are you sure the resulting bundle from typescript really is an ES Module or just something that has been transpiled to ES5 and made to mostly act like one? I would like to see an example project that this applies to, so that it's possible to look at the code under test after transpilation. The added code does not really give me confidence we are actually talking about ESM. In which case, the test before it, isEsModule() should have returned true. Willing to learn, of course 😄

[FauxFaux]

I had left this in draft because I didn't, and still don't, really know how to write a reasonable test for it.

I've pushed a test which includes some typescript source code, a Makefile which uses npx to generate the JS (generated code committed), and a test which shows the problem, and asserts that the new feature is there.

The "silently fails to stub" test shows the problem, and passes (i.e. demonstrates the bug exists) without any of my other code changes.

This breaks npm t, because the test runs on Chrome and uses process.env. The proper fix for this is probably just to remove the conditional code, and always throw?

As to whether these things are es modules or not, I do not know. The Typescript team generally deviate from the spec, except for where it suits them, and they have:
a) named the things they generate __esModule, in line with a number of other conventions, and
b) made this breaking change in the name of spec compliance

It's certainly not something I've ever had to care about, as a node/commonjs/JS/TS/babel/webpack user, except for when my tests start hanging without any feedback.

[FauxFaux]

The problem isn't really that it's a getter, or that it's a module. It that it's an intermediate reference, right?
We could handle getters, by simply deleting them. It doesn't generate getters for regular exports, only for intermediate exports like this.

If you mock the real module it's fine, for which I've added an extra test. And, this is what typescript users would need to do, and I want to enforce across codebases where I'm trying to get people to use sinon and typescript together.

[fatso83]

It's certainly not something I've ever had to care about, as a node/commonjs/JS/TS/babel/webpack user, except for when my tests start hanging without any feedback.

Generally, Sinon is not concerned with module loading in any form (CommonJS, UMD, RequireJS, webpack, whatever), leaving that to dedicated products such as proxyquire and similar, as the process is different for every project and in the case of module bundlers, like WebPack, different between each major version, even. That lets us avoid a whole bunch of issues and maintenance hell. We leave that to the users who create and maintain their own stack to figure out.

From a little bit of (quite shallow) research, I found that the __esModule field is a convention used by Babel (which probably does your transpilation) to hint to the bundler/module loader what the default export is. It is not an ECMAScript Module (which is a runtime feature), but a synthetic approximation of its behavior that works in non-supporting environments (like Node). So if we leave the world of TypeScript, Babel and ES Module for a bit, let's just look at what we know:

• we have an object whose properties (the "exports") are_ implemented as getters/setters, not values
• if you try to stub that using Sinon's normal 'stub' mechanism you get a silent error
• silent errors are annoying

Getters and setters are not a problem per se for Sinon. They can be both spied and stubbed. Of course, for that to happen, the property needs to be configurable and no one has used Object.freeze() or similar. configurable is false by default in all your code examples, so none of them are "stubbable" in that sense.

So the gist of all this code is some way of notifying the user explicitly that this code is not stubbable, am I right? We have tried to avoid logging stuff like this in Sinon for the most part, so I am not totally confident about what to do here. In any case, the code mentioning TypeScript and ES Module's are a bit superflous, as they are just specific instances of the more general problem, AFAIK, and can be dropped.

[fatso83]

Instead of all of this, I would just drop the esModule thing, check if the prop had a getter and setter (like you do) and if these were configurable. If no on the latter, we could go into the switch block. Otherwise, if for some reason Babel in the future supported exporting configurable props you could still modify the props using Sinon's getter/setter functionality.

[fatso83]

P.S. I still think the right approach here is to write a Babel plugin that one can configure to export objects with plain props for the sake of stubbing, possibly based upon something like plugin-proposal-export-default-from.

That would enable you to just add that plugin to your custom Babel config that extends the normal one and have a simplified test setup.

[fatso83]

Hi, again. Maybe what I mentioned above already exists in the form of the loose config ... care to check it out for your use case?

I was browsing the source for babel-plugin-transform-modules-commonjs and came across this, which is what creates the property definitions. Passing in loose changes the resulting code.

Well worth a shot, IMHO, if it helps keep Sinon slim and lets you avoid writing a Babel plugin :) You can probably just create a test config /test/babel.config.json that references your existing one like this:

{                                                                                                       
  "extends": "../babel.config.json",
   "// your overrides here": true
  ["transform-es2015-classes", {"loose": true}],
}

(Can't help with the actual config, alas, but the general concept would be something like it)

[FauxFaux]

There's no babel in my stack, these files are what the typescript compiler outputs. The committed example JS is compiled directly from the committed example TS with no babel. My comments above were about how the typescript compiler seems to have picked some convention (which we're calling "the babel convention"), yes.

I have another (although maybe much worse) babel solution ,where I just delete the getters, but I'm hoping not to need it long term, by fixing all my tests. I'm using this patch to diagnose tests, and have raised it here so others can benefit.

[fatso83]

Thank you for your work. As mentioned, I think it would be more useful if not tied down to ESM being transpiled by bundlers and attack the general issue of "un-stubbability". I summed it up in a new issue: #2377

Closing this for now, but some of the code has a high chance of being reused in some form to fix #2377.

Feel free to re-submit it in a form that would fix #2377 as that would help in many other cases, as well!