Support testEnvironment=jest-environment-jsdom with pnpm

[solsson]

Summary

The dynamic require of testEnvironment (transpiled to const testFramework = require(config.testRunner) in published source) is incompatible with pnpm.

I agree with pnpm/pnpm#1130 (comment).

For environment to be truly pluggable, with stricter npm install, I guess this PR needs to consider how to support environments other than jsdom. It could be argued that a class should be injected, rather than required using a runtime value. In practice though I think the suggested addition to package.json solves the problem.

Test plan

Fixes #5369

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[codecov-io]

Codecov Report

Merging #6145 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6145   +/-   ##
=======================================
  Coverage   64.19%   64.19%           
=======================================
  Files         219      219           
  Lines        8423     8423           
  Branches        4        4           
=======================================
  Hits         5407     5407           
  Misses       3015     3015           
  Partials        1        1

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ca03cd...0f1d392. Read the comment docs.

[SimenB]

If we do this, it should also include jest-environment-node, and remove them from jest-config.

Also please update the changelog

[SimenB]

Sorta related #5913

[SimenB]

Another possibility is to do require.resolve in jest-config (and the whole "try with jest-environment prefix" logic), and pass the full path to the runner.

@cpojer @thymikee @rickhanlonii thoughts? I'd also like to not paint us into a corner with regards to #5913 (maybe even solve it at the same time)

[octogonz]

This issue is affecting my application also, because we have to install two versions of Jest side by side. In our specific case, the jest-runner@21.2.1 ends up requiring jest-environment-jsdom@22.4.3 which causes this error:

TypeError: environment.dispose is not a function

PNPM brings the problem to light, but this is fundamentally an architectural problem with Jest not just a compatibility issue: In the modern world, libraries cannot naively call require(X) to fish a dependency out of the node_modules folder without any idea of what version of X they will get. The version needs to be constrained somehow, either directly via package.json as a dependency, or indirectly via a peer dependency or injection.

The approach suggested in this PR would help the simple case where you get jsdom as your default. However the jsdom dependency is like 11MB with all its dependencies. Forcing everyone to install that and probably also jest-environment-node seems contrary to the goal of modeling the environment as a swappable plug-in. It also doesn't help advanced users who have custom packages.

It also poses a backwards compatibility problem: If jest-environment-jsdom is a peer dependency, upgrading consumers will fail to install unless they satisfy a new peer. Or if it's a regular dependency, consumers may get an unexpected version if the node_modules tree gets shuffled around.

A better solution would be to make options.testEnvironment an injectable instance instead of a simple text string. This is how other libraries treat their plugins, and it is the architecturally sound way to specify a plugin.

[octogonz]

I tried creating a PR that would illustrate how to allow something like this:

// jest.config.js
module.exports = {
  verbose: true,
  testEnvironment: require('my-custom-environment'),
};

or for jest.config.json

{
  "verbose": true,
  "testEnvironment": "../node_modules/my-custom-environment"
}

However I cannot seem to get the master branch of Jest to build at all, on Windows or Mac. The yarn install keeps failing with various errors...

[solsson]

@SimenB Would you be willing to merge this fix, or @pgonzal's fix, as an intermediate solution?

[SimenB]

Doing require.resolve from within jest-config and pass a full path through to the runner might make the most sense. However, this change should work, and is better than nothing

[cpojer]

Leaving this one for @arcanis to merge.

[thymikee]

@arcanis friendly ping :)

[arcanis]

You cannot remove them from here, because they are required by the following line:

https://github.com/facebook/jest/blob/master/packages/jest-config/src/utils.js#L123

If you remove them from the package list, you start relying on the hoisting to make sure that the environments are siblings of jest-config, which is an unsafe assumption.

[solsson]

Looks like a require based on a runtime variable is done in both jest-runner and jest-config. I guess that has implications on @SimenB's suggestion "pass a full path through to the runner" too.

I suppose that leaves us with the option of some refactoring towards dependency injection, and maybe @pgonzal's suggestion with a require done at config time is a simple way to do that.

[thymikee]

@solsson mind taking a look at @arcanis #6145 (comment)?

[thymikee]

It's been a month already without a sign of interest, closing. Happy to reopen once the feedback is addressed :)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.