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

Support ESM versions of all pluggable modules #11167

Open
12 of 16 tasks
SimenB opened this issue Mar 7, 2021 · 26 comments
Open
12 of 16 tasks

Support ESM versions of all pluggable modules #11167

SimenB opened this issue Mar 7, 2021 · 26 comments

Comments

@SimenB
Copy link
Member

SimenB commented Mar 7, 2021

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
Copy link
Contributor

ahnpnl commented Mar 9, 2021

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

@SimenB
Copy link
Member Author

SimenB commented Mar 9, 2021

sure 🙂

@SimenB SimenB pinned this issue Mar 9, 2021
@SimenB SimenB added this to the Jest 27 milestone Apr 22, 2021
@theoludwig
Copy link
Contributor

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. 👍

@ahnpnl
Copy link
Contributor

ahnpnl commented Apr 30, 2021

D-Day for Node 10 is here xD

@SimenB
Copy link
Member Author

SimenB commented Apr 30, 2021

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

@gilles-yvetot
Copy link
Contributor

@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
Copy link
Member Author

SimenB commented May 4, 2021

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

@gilles-yvetot
Copy link
Contributor

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
Copy link
Member Author

SimenB commented May 4, 2021

Could you open up a new issue with a reproduction?

@thernstig
Copy link
Contributor

thernstig commented May 26, 2021

Should moduleNameMapper be part of this meta-task?

@SimenB
Copy link
Member Author

SimenB commented May 26, 2021

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
Copy link
Contributor

I tested it, and it does work 😆

@chentsulin
Copy link
Contributor

chentsulin commented Oct 28, 2021

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

  • dependencyExtractor
  • prettierPath
  • snapshotResolver
  • snapshotSerializers

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
Copy link
Member Author

SimenB commented Oct 28, 2021

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Oct 28, 2021

That should work as well 👍

@F3n67u
Copy link
Contributor

F3n67u commented Apr 24, 2022

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

@SimenB
Copy link
Member Author

SimenB commented Apr 24, 2022

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

@jestjs jestjs deleted a comment from 337Gslime Apr 24, 2022
@damianobarbati
Copy link

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

@RanylFoumbi
Copy link

RanylFoumbi commented Apr 25, 2023

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 axios/axios#5676 (comment)

@carlocorradini
Copy link

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

@Anutrix
Copy link

Anutrix commented Sep 21, 2023

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
Copy link

🎻

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

No branches or pull requests

16 participants