Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transformSync and jest-mock #412

Closed
flunderpero opened this issue Sep 24, 2020 · 23 comments
Closed

transformSync and jest-mock #412

flunderpero opened this issue Sep 24, 2020 · 23 comments

Comments

@flunderpero
Copy link

We try to use esbuild as a transformer for Jest using the cjs format. All works fine but we cannot use jest.spyOn() on any of the transpiled modules. Jest complains with TypeError: Cannot set property foo of #<Object> which has only a getter, which is correct, because esbuild only defines getters for exported symbols, like in __export:

var __export = (target, all) => {
  __markAsModule(target);
  for (var name in all)
    __defineProperty(target, name, {get: all[name], enumerable: true});
};

Typescript exports symbols by simply assigning them to exports, like this:

function foo() {
    ...
}
exports.foo = foo;
@zmitry
Copy link

zmitry commented Sep 25, 2020

Hey, could you share your setup, looks interesting?

@evanw
Copy link
Owner

evanw commented Sep 25, 2020

I believe esbuild's behavior is correct here because ECMAScript module exports are supposed to not be writable. From the section on "module namespace exotic objects" in the specification:

9.4.6.8 [[Set]] ( P, V, Receiver )

When the [[Set]] internal method of a module namespace exotic object O is called with property key P, value V, and ECMAScript language value Receiver, the following steps are taken:

  1. Return false.

This matches the actual behavior that you get in node when you try to assign to a property on a module namespace:

// export.mjs
export let foo = 123
// import.mjs
import * as ns from './export.mjs'
ns.foo = () => console.log('mock')

If you run this code with node --experimental-modules import.js you will also get a TypeError:

$ node --experimental-modules import.js
ns.foo = () => console.log('mock')
       ^

TypeError: Cannot assign to read only property 'foo' of object '[object Module]'
    at import.mjs:3:8

@flunderpero
Copy link
Author

Hi @evanw,

this is all correct. My intent was not to point out something wrong with esbuild (it is great, we use it for everything at cling.com - we bundle our web_app and we transpile all of our Node code with esbuild). We have a huge test-base that would greatly benefit from esbuild's speed (even though ts-jest in transpile mode is not that slow). In the end we just want to use a single build tool, so that we actually test exactly what goes into production.

You are correct wrt ECMAScript modules. But my target is cjs not esm. Jest itself still has work to do to fully support modules. And the biggest blocker seems to be mocking of ES modules.

To illustrate what I am trying to do, I created a small sample repo at https://github.com/flunderpero/esbuild-jest-example

You can check it out and run the tests with either ./node_modules/.bin/jest --config jest.esbuild.config.js test or ./node_modules/.bin/jest --config jest.config.js test to see the difference.

I would suggest that esbuild should mimic what tsc does when it comes to transpiling Typescript to JS.

@flunderpero
Copy link
Author

@evanw Are you considering this?

@cem2ran
Copy link

cem2ran commented Oct 10, 2020

Running into the same issue. Getting this working and replacing ts-jest would be immensely useful & appreciated! 🙏

@FabianSellmann
Copy link

Could we argue that, by specifying cjs format that we do not expect es module behavior? Or would we need to have another format to differentiate between the case where a user wants to have their code work in a commonjs environment, but keep the behavior they would expect when they write their code using ES modules, versus explictly transpiling the code into something that resembles other/older transpilers (e.g. adding a setter or using data descriptors for exports, specifically for mocking). Or is setting format generally just expected to change only the format but not the behavior?

@evanw
Copy link
Owner

evanw commented Oct 12, 2020

Or is setting format generally just expected to change only the format but not the behavior?

Yes, that's correct.

@evanw Are you considering this?

Not at the moment, no. There are a lot of current priorities and this is low priority for me given that this wasn't the intended use case for esbuild and it's not a simple change.

Something like swc might be a better fit for this. I did an initial test and it looks like their CommonJS transform doesn't follow ECMAScript module semantics and might work with Jest like this.

@flunderpero
Copy link
Author

@evanw I was toying around with this and I agree that it is not a simple change.

If the only use case is mocking with Jest atm, I agree that it should be low priority. I wonder if there is any library out there doing some monkey patching on common/standard libs and would not work out-of-the-box. I don't know of anything like that and perhaps it's just me stretching to find another use case. :-)

Feel free to close this issue.

@evanw
Copy link
Owner

evanw commented Nov 6, 2020

FWIW I'm currently investigating an esbuild-compatible alternative to a similar testing framework, although it wasn't Jest specifically. I came up with a helper function that looks something like this (not actually tested with Jest):

export interface HasSpyOn {
  spyOn(): jest.SpyInstance
}

export function enableSpyOn<T extends Function>(fn: T): T & HasSpyOn {
  if (TEST) {
    let name = fn.name, obj = {[name]: fn};
    (fn as any) = function(this: any) { return obj[name].apply(this, arguments) };
    (fn as any).spyOn = () => jest.spyOn(obj, name);
  }
  return fn as any;
}

Instead of doing this:

// file.ts
export function fn() {}

// file_test.ts
import * as file from './file'
let spy = jest.spyOn(file, 'fn')

You should be able to do this instead:

// file.ts
import {enableSpyOn} from 'helpers/enableSpyOn'
export const fn = enableSpyOn(function fn() {})

// file_test.ts
import {fn} from './file'
let spy = fn.spyOn()

With esbuild you would define TEST to true when running tests and to false otherwise. I haven't fully integrated it yet but this approach seems promising.

@KnisterPeter
Copy link

With esbuild you would define TEST to true when running tests and to false otherwise. I haven't fully integrated it yet but this approach seems promising.

That would mean you need to compile your code twice. Once for testing and once for production. And since both are different (e.g. the test build will contain jest while the production build will tree-shake that) how can one be sure that it doesn't break.

There might be side-effects in imports which where tree-shaked in the production build I think.
For example if jest is doing things like this somewhere in the code:

// this will be a side-effect
import "some-file";

So the option here would then be, to ship jest to production as well, or to live with the knowledge that things might break.

@shigma
Copy link

shigma commented Feb 16, 2021

Hi @evanw

I believe your implementation for es module is all correct, but I think it should not be used as an implementation of the platform=node:

If you are bundling code that will be run in node, you should configure the platform setting by passing --platform=node to esbuild. This simultaneously changes a few different settings to node-friendly default values. (https://esbuild.github.io/getting-started/#bundling-for-node)

If the code generated by esbuild cannot be run smoothly in node, how can we call it "node-friendly"?

Maybe a different set of helper functions should be applied when platform is set to node.

@evanw
Copy link
Owner

evanw commented Feb 16, 2021

If the code generated by esbuild cannot be run smoothly in node, how can we call it "node-friendly"?

This is happening because esbuild's bundler is respecting the semantics of the original code. The underlying problem is that Jest's spyOn function is a CommonJS-only API. It doesn't work with ESM because module exports in ESM are immutable. This applies both when running tests in node and when running tests bundled with esbuild, since esbuild emulates node's module semantics. I made an example to demonstrate this: https://github.com/evanw/jest-esbuild-demo.

Being "node friendly" only means that code which works in node when it's not bundled also works in node when it's bundled with esbuild. It doesn't guarantee that code which doesn't work in node without being bundled will work in node after being bundled with esbuild.

It is possible to use Jest with esbuild. You just need to write this:

const sut = require("../sut.js");

instead of this:

import * as sut from "../sut.js";

since Jest's spyOn function is a CommonJS API and only works with CommonJS modules. Another way to say this is that esbuild supports input files in both CommonJS and ESM format, but it does not support input files in looks-like-ESM-but-is-actually-CommonJS format.

@jhiode
Copy link

jhiode commented Feb 19, 2021

Couldn't we introduce a new output format (eg cjs-compat or cjs-transform) that would transform esm to "classic" cjs instead of keeping the semantics of esm?

@DerGernTod
Copy link

this whole topic is very interesting to follow. i like the approach that esbuild is strictly implementing the specification (thus, esm being immutable). it's unfortunate that jest is not compliant with that.

would be nice if that workaround worked, but for me it doesn't:

It is possible to use Jest with esbuild. You just need to write this:

const sut = require("../sut.js");

instead of this:

import * as sut from "../sut.js";

i still get the same "cannot redefine property" error that i got with the esm approach. thing is, i mixed imports, using commonjs instead of import * as ... from "..." so i can use spies, and esm for other imports. i can't transform the whole module into commonjs (doesn't play well with generic type-only imports and module augmentation). any other recommendations?

@akshayr20
Copy link

@evanw I was able to find a fix/workaround on the above issue. If you could review it, it would be helpful.

Background:
We can only spy on a method in an object.
Syntax: jest.SpyOn(object, methodName); learn more here
Example:

  // fileName: feature.ts
  const fetchUserFn = () => // fetch User by making an API call

 // fileName: feature.test.ts
  import * as API from './feature.ts';
  test('something', () => {
    const spyFetchApi = jest.spyOn(API, "fetchUserFn");
    ...
  });

The syntax import * as API from './feature.ts'; is importing all the modules from the feature file and wrapping the same in an object (Not a real object but a namespace)

When we use the syntax jest.spyOn(API, "fetchUserFn") tries to overrides the method but fails because ECMAScript module exports are supposed immutable. (Learn more here)

Although using spies is a valid use case from a testing point of view, handling the same within esbuild seems invalid.

I found a workaround to address the above issue.
If we change the export from primitive to non-primitive, it should ideally work.
The fundamental difference between primitives and non-primitive is that primitives are immutable, and non-primitive are mutable. IF we expose fetchUserFn in the above scenario by wrapping in an object from the feature.test.ts file mutating the same will be straightforward.

Please find code snippet for fix/workaround here.

@neutraali
Copy link

neutraali commented Oct 20, 2021

Dunno whether this is a similar issue, but there are many third party libraries that shim jQuery -functions as well, as an example:

$.isFunction = $.isFunction || function(obj) {
  return typeof obj === "function" && typeof obj.nodeType !== "number";
};

^ The above will crash in a similar fashion with:
Cannot set property isFunction of [object Object] which has only a getter

EDIT: This particular scenario is produced by trying to inject jQuery globals

@evanw
Copy link
Owner

evanw commented Oct 24, 2021

It works fine for me:

import $ from 'jquery'

$.isFunction = $.isFunction || function (obj) {
  return typeof obj === "function" && typeof obj.nodeType !== "number";
};

console.log($.isFunction + '')

Make sure you use import $ from 'jquery' or let $ = require('jquery') not import * as $ from 'jquery'. The first two return the actual jQuery object while the third returns a module namespace exotic object which is read-only. That's how node's CommonJS module system works, so that's how esbuild's CommonJS module system works too.

@sastraxi
Copy link

This whole conversation is fascinating, and we're now coming across this issue as we attempt to speed up some of our frontend tooling by moving from webpack/babel to esbuild.

Suppose one was writing a greenfield project -- is using jest the wrong approach if we want to keep things modern and use only esmodules? If so, what's the alternative? My understanding is pretty thin at the moment, but it doesn't seem like any sort of spying/mocking library would be compatible with the fact that ECMAScript module exports are supposed to not be writable.

@flunderpero
Copy link
Author

Even though I initially raised this issue, we since then removed all of our jest.spyOn-code on module level to use proper jest.mock to mock out the whole module like this:

jest.mock("module", () => {
    const mod = jest.requireActual("module")
    return {
        ...mod,
        random_uid: jest.fn(),
    }
})

It is not as convenient as simply using jest.spyOn, but jest.spyOn will never work with ECMAScript modules and it feels "a bit cleaner". We have a quite large code- and test-code-base, but the switch was not too hard, a day at most. And that day was very well spent because switching to esbuild gives our developers so much in development speed that it was worth it.

Currently, we are not using ESM in our tests because Jest still has some issues with that.

@evanw I am fine with closing this issue.

@dave-irvine
Copy link

@flunderpero are the modules you're mocking ESM or CJS? And are they your own code or external modules from node_modules/?

I cannot get jest.mock() to work with my own ESM code. The general advice (jestjs/jest#10025) seems to be to transpile from ESM to CJS, and most guides are for babel. I'm trying the same thing with esbuild but without luck, possibly because the "hoisting" that usually happens with babel isn't happening here?

@evanw
Copy link
Owner

evanw commented Jan 21, 2022

jest.spyOn will never work with ECMAScript modules

My previous comment contains a workaround for jest.spyOn and ESM in case it helps: #412 (comment). Figma recently switched to esbuild and I believe they made use of this workaround in all of their tests, which worked out fine. The function call even disappears entirely in production builds due to a recent change in version 0.14.10 that inlines calls to identity functions.

@evanw I am fine with closing this issue.

Makes sense. This is a problem with Jest, not with esbuild. Closing.

@mctrafik
Copy link

Can you share an example of what figma did? Would love to inspect code because I couldn't see a resolution in this thread.

@evanw
Copy link
Owner

evanw commented Apr 26, 2023

I can’t speak for Figma because I don’t work there. This comment documents what I was exploring when I did still work there, however: #412 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests