jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.21k stars 6.46k forks source link

Support ESM versions of all pluggable modules #11167

Open SimenB opened 3 years ago

SimenB commented 3 years ago

Once Node 10 is EOL at the end of April, lots of libraries and modules will probably be written in native ESM rather than CJS. While we've mostly been focusing on getting tests running in ESM, we're sorta limited/slowed down by the fact the vm APIs are experimental and flagged on Node's side. That is not the case for "normal" ESM.

We already support user configuration written in ESM.

With that in mind, all modules in Jest that are "pluggable" should be able to load ESM.

Any and all help here would be greatly appreciated!

In general, it means attempting to require, and if that fails with an ESM error, use import() and verify it's the default export. Paired with an integration test that has the module in question written in ESM and verifying it works.

Example: https://github.com/facebook/jest/blob/ab014c140af2e10ae7c5790c2406009790787cdb/packages/jest-transform/src/ScriptTransformer.ts#L177-L196


Whenever we drop Node 10 (probably for Jest 28) we can do just the import call as that can load both ESM and CJS, but that'll be a simple refactor as it's just removing the initial require and try-catch. So I still think it's worth it to add support now as the code difference is quite small and the later refactor is minimal

ahnpnl commented 3 years ago

I think it’s wise to pin this issue so it stands out to everyone :)

SimenB commented 3 years ago

sure 🙂

theoludwig commented 3 years ago

I think we can already drop the support node 10 for jest@27, there is no need to wait for jest@28 as it will reach EOL. :+1:

ahnpnl commented 3 years ago

D-Day for Node 10 is here xD

SimenB commented 3 years ago

Jest 27 will support Node 10 so people who are stuck on older versions (of which there are many) can use it

gilles-yvetot commented 3 years ago

@SimenB you listed globalSetup as one of thing to migrate to ESM but I don't see setupFiles. Is it an omission or it will be taken care with globalSetup?

SimenB commented 3 years ago

setupFiles are part of the test run, so that already works 🙂

gilles-yvetot commented 3 years ago

Maybe that's one of the modules I am using but I cannot use ESM in my setupFiles with the current version 27.0.0-next.9

SimenB commented 3 years ago

Could you open up a new issue with a reproduction?

SimenB commented 3 years ago

I want to do reporters and resolver, then release v27. Anyone up for sending PRs for those?

akauppi commented 3 years ago

Maybe that's one of the modules I am using but I cannot use ESM in my setupFiles with the current version 27.0.0-next.9

Hi @gilles-yvetot

If you want to fix the setupFiles thing, you can compare your approach to what I'm doing in firebase-jest-testing. It uses both Jest 27.0.0-next.9 and ESM globalSetup.

SimenB commented 3 years ago

Decided to skip resolver for now, as I think having to make it async will probably be more natural together with adding async resolution in #9505.

Opened up #11427 for reporters

thernstig commented 3 years ago

Should moduleNameMapper be part of this meta-task?

SimenB commented 3 years ago

Hmm, maybe. I'd have assumed that worked as it should seeing as it should just remap the identifier and the rest should work as "normal". Is that not the case?

thernstig commented 3 years ago

I tested it, and it does work 😆

chentsulin commented 2 years ago

Hi @SimenB, I just try to help doing these parts:

After I dived into it, I found there're some problems that I can't solve by myself.

dependencyExtractor

dependencyExtractor is used in the constructor:

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-haste-map/src/index.ts#L241-L250

So the only chance I could find is to make the whole HasteMap factory async like this:

await HasteMap.create(...);

Is this a right approach?

prettierPath

prettierPath is required using a helper called requireOutside:

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-snapshot/src/InlineSnapshots.ts#L53-L60

This requireOutside function will be transformed by babel-plugin-jest-require-outside-vm:

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/scripts/babel-plugin-jest-require-outside-vm.js#L13-L21

I currently don't have enough knowledge about how to handle this with ESModules.

snapshotResolver & snapshotSerializers

Those two are relying on the localRequire function:

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-snapshot/src/SnapshotResolver.ts#L79-L85

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-jasmine2/src/setup_jest_globals.ts#L95-L104

So, should we pass runtime.unstable_importModule.bind(runtime) as localImport to solve this problem?

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts#L34

https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-runtime/src/index.ts#L622

SimenB commented 2 years ago

Wonderful, thanks!

dependencyExtractor

Yeah, that seems correct. Makes it a breaking change though, so need to wait for Jest 28.

prettierPath

Good point, we might have to leave that alone. This is for a specific module (prettier) though, so not as likely that people plug in their own thing which is ESM. If (when) prettier migrates to ESM only, we must change, but up until that leaving it as require seems sensible.

snapshotResolver & snapshotSerializers

Yeah, passing in unstable_importModule makes sense. Should probably also pass in https://github.com/facebook/jest/blob/3d04f33daa3b9ec21838c9177ab7acab8c983155/packages/jest-runtime/src/index.ts#L421

chentsulin commented 2 years ago

Passing localRequire, unstable_importModule and unstable_shouldLoadAsEsm through multiple layers looks a little bit tricky and breaks function signature unavoidably.

How about replacing localRequire with a localRequireOrImportModule function?

SimenB commented 2 years ago

That should work as well 👍

F3n67u commented 2 years ago

@SimenB is there any work left to do? I am very glad to push these things forward.

SimenB commented 2 years ago

Great! The ones not crossed out in the OP are still missing. Not sure how feasible they are

damianobarbati commented 2 years ago

@SimenB is this waiting for nodejs team to provide support?

RanylFoumbi commented 1 year ago

HI @SimenB i'm currently facing badly issue using jest ES6 to mock axios in nodejs jest doesn't mock axios like when using ESM like it was when using require to import module Please take a look here https://github.com/axios/axios/issues/5676#issue-1682834041

carlocorradini commented 1 year ago

Is it possible to make expect ESM compatible? ESM compatibility is worthwhile since it can be installed as a separate module/dependency.

Anutrix commented 1 year ago

Good point, we might have to leave that alone. This is for a specific module (prettier) though, so not as likely that people plug in their own thing which is ESM. If (when) prettier migrates to ESM only, we must change, but up until that leaving it as require seems sensible.

Prettier seems to have moved on to ESM: https://prettier.io/blog/2023/07/05/3.0.0.html.

usrrname commented 1 year ago

🎻