Issue mocking exports via jest - inconsistency with Babel behaviour

[jdb8]

Hi there, first off - thanks for making this library! I've been playing around on a large existing codebase comparing timings with Babel and it looks really promising.

I've run into something strange when comparing Babel and Sucrase for a few cases in Jest. I realise this may end up being a case of "don't do what you're doing in your test", but I think it's worth noting that Babel appears to be fine with the code.

It's kind of hard to give a full explanation given that I don't yet understand what's going on, so I've made a repro here which should demonstrate a minimal example of what I'm talking about: https://github.com/jdb8/sucrase-jest-export-repro.

The issue seems to be that Babel somehow transpiles module exports in a way that makes this kind of transitive mocking/spying work, while Sucrase does not. I don't know if that means Babel or Jest are actually doing something wrong rather than Sucrase, but it's unfortunate as it limits my ability to hotswap our existing Babel transforms for Sucrase ones in many cases.

---

A related but similar case which felt easier to fix in the test itself was one where we were mocking the imported function like this:

import * as Foo from 'some-foo-library';

Foo.thing = jest.fn();
Foo.anotherThing = jest.fn();

Something about the difference between Babel and Sucrase also broke this code in a way I didn't fully understand, but I switched over to using jest.mock since arguably assigning things to exports like this isn't the right way to do things.

---

Overall, both of these cases make me think that the way these exports are transpiled differently somehow matters: under babel exports seem to produce something like this:

Object.defineProperty(exports, "getValue", {
  enumerable: true,
  get: function () {
    return _someLib.default;
  }
});

vs exports.getValue = _somelib2.default;

Would be very curious to hear your thoughts, since I think I'm missing some of the nuances around how both Babel and Sucrase treat this code, and why they take different approaches (and why that seems to matter here!). Thanks in advance for taking a look!

[alangpierce]

Hi @jdb8 , thanks for the detailed report! Getting this sort of thing working is definitely in scope for Sucrase, even though, as you mention, some people might argue that you shouldn't be writing tests like this. At my company (where we use Sucrase for local dev and Babel/TypeScript in CI and production), we also have plenty of test code that directly patches module objects like you're describing.

I think I have a pretty good handle on what's going wrong, at least for your first case, and I have some theories about the second case. Happy to explain what's going on. It's likely that there's a reasonable fix to make Sucrase work seamlessly here, though I'd need to think a little more about implementation details and backcompat concerns.

To explain the issue, it's good to first understand how ES modules (import/export syntax) are supposed to work according to the spec. You can test all this out by trying a real ESM project in a recent version of Node. Most notably:

• Imported and exported values are always live bindings. The best way to think about this is that it's as if the imported variable is directly referencing the variable in the other file. If you have export let foo = 1; in one file and import {foo as bar} ... in another file, then all usages of bar need to read the latest value of foo. Even for const declarations, this sort of lazy-access is useful to avoid problems with circular imports.
• The module object (the thing you get from import * as Foo or await import('some-foo-library')) cannot be modified externally. No adding new properties, no setting/defining existing properties. You could almost call it "immutable", except that the underlying values can change due to the live binding behavior described above.

(There are other details with the spec like async loading and requiring file extensions, but those aren't relevant here.)

That second bullet point means that your code would break when switching to true ES modules, since Foo.thing = jest.fn(); and jest.spyOn(SomeLib, 'default'); are both trying to modify a module object. Maybe we should all stop writing tests like that, but IMO the pattern is valuable enough that the community should come up with ways to make that style of mocking/patching work. I have some ideas of my own, basically using a transpile step to switch const to let and to expose setters for patching, and I'm hoping to explore it further at some point.

The first bullet point (live bindings) is the main challenge that Babel, TypeScript, and Sucrase need to deal with when converting import/export syntax to CommonJS (require/module.exports). The main consequence is that import {x} can't just save the value to a variable named x, we need to transform all usages to access the latest value of x from the other module. It also means that when exporting a variable, we need to make sure the module value is updated as well, e.g. x = 1; needs to become exports.x = 1; (how TS does it) or exports.x = x = 1; (how Babel does it). In most cases, Sucrase is closer to TS in the implementation details, but in the past I've gone back and forth on the ways to do it.

For more complex scenarios like import-and-export, the tools diverge a bit more, but it's probably fair to say that Babel is more correct. Babel defines a computed getter that reads the latest value from the other module, while TypeScript and Sucrase just access the value at import time (i.e. it's not actually a live binding). It looks like TS behaves differently when re-exporting a default import vs re-exporting a named import, even though both should ideally have live bindings.

Sucrase actually does do the right thing if you change the re-export to look like this:

export {default as getValue} from './some-lib';

so you could try that as a workaround and see if that helps.

The way to get your code working fully in Sucrase is for Sucrase to recognize that the export is from an import and write it to use a getter instead. This could possibly cause issues with tests assuming that the export is editable, so I'm a little hesitant about backcompat concerns, but I'd need to think about it; it might be fine.

With your some-foo-library example, my best guess is that if some-foo-library is also transpiled, then it might be a difference between Object.defineProperty vs just setting a property on exports for those values that you're patching. But it's hard to say for sure.

[jdb8]

@alangpierce thank you for the very detailed explanation! I'll try out what you suggested.

I was also able to make things work in the specific case I ran into by updating the code to import from the same place we were trying to mock, but I'm imagining that in some places that may not be easy or possible.

Good point re:ESM, it's one of the potential future blockers to switching over so this may be a good forcing function to update some tests that are a little too reliant on transpiled behaviour.

Definitely interested if there's something that can be done inside sucrase to make things magically work, though! But this stuff seems pretty complex so I understand if it's too risky :)