auto cleanup will break down testing in mocha & esm environment

[whodoeshua]

• @testing-library/react version: 9.5.0
• react version: ^16.12.0
• node version: 12.13.0
• mocha version: ^6.2.2,
• mochawesome version: ^4.1.0,
• esm version: ^3.2.25
• npm (or yarn) version: yarn@1.17.3

Relevant code or config:

Github demo as below:

https://github.com/whodoeshua/mocha-react-testing-library

package.json

  {
"scripts": {
    "test": "tsc --pretty && mocha --exit --trace-warnings -r source-map-support/register -r jsdom-global/register -r esm -c \"build/**/@(skip|test)-*.js\""
  },
}

What you did:

yarn test

What happened:

Global afterEach hook will beak down testing with timeout error, cause of

1) "after each" hook for "test":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at listOnTimeout (internal/timers.js:531:17)
      at processTimers (internal/timers.js:475:7)

Reproduction:

https://github.com/whodoeshua/mocha-react-testing-library

Problem description:

This problem is cause of auto cleanup. Auto clean up is an async funtion and required timers module to make a fake timer. But require function will throw out a error once esm module has been required. And the async cleanup will never stop.

Auto cleanup will call flush-microtasks.

// @testing-library/react/src/index.js:9
if (typeof afterEach === 'function' && !process.env.RTL_SKIP_AUTO_CLEANUP) {
  afterEach(async () => {
    await flush()
    cleanup()
  })
}

require function in flush-microtasks will throw out an error.

// @testing-library/react/src/flush-microtasks.js:18
  enqueueTask = nodeRequire('timers').setImmediate

[eps1lon]

You can skip autocleanup by either setting the environment variable RTL_SKIP_AUTO_CLEANUP or importing from @testing-library/react/pure. But for proper cleanup running async functions is a hard requirement.

[kentcdodds]

I think the goal is to make it so you can autocleanup when using esm. Pretty sure this should remain open.

[whodoeshua]

I think what I want is not remove async cleanup in testing but make sure nodeRequired could run in esm environment. Otherwise I think throw out an error to tell developer what is happening is better than catch it and just waiting for timeout. That will be a problem once I set up a long timeout value.

[kentcdodds]

Yeah, I'd like to figure out how to make that nodeRequire use case work for esm.

I don't have bandwidth to work on it though. @whodoeshua, do you have time to dive in?

[eps1lon]

Oh now I see the issue. This only happens on node 12.13 but not prior versions, right?

I guess we can switch to a proper import() and see how these are transpiled to commonJS modules.

[eps1lon]

I think we need to be careful with this change. It seems like we're using require so that jest can reset the modules properly. You don't need to require('timers') usually since these are available on the global object:

Because the timer functions are globals, there is no need to call require('timers') to use the API.

-- https://nodejs.org/docs/latest-v12.x/api/timers.html

[whodoeshua]

I find a problem that makes me very confused.

I use four way to required timer as below and only the way that using currently will failure:

  const requireString = `require${Math.random()}`.slice(0, 7);
  console.log(requireString)
  const nodeRequire = module && module[requireString]; // assuming we're in node, let's try to get node's
  // version of setImmediate, bypassing fake timers if any.
  enqueueTask = nodeRequire('timers').setImmediate; // failure
  enqueueTask = require('timers').setImmediate; // success
  enqueueTask = module.require('timers').setImmediate; // success
  enqueueTask = module['require']('timers').setImmediate; // success

And I'm confuse with the comments in flush-microtasks.js currently. Is there any different with use require?

[kentcdodds]

@threepointone is the one who wrote this code and knows the most about it. I'm pretty sure he got it from React source (which he may have written as well, I'm not sure).

[eps1lon]

And I'm confuse with the comments in flush-microtasks.js currently. Is there any different with use require?

The failure case is likely the only one that isn't transpiled by webpack and uses the actual require() from node not the bundled webpack_require. require() throws if used in a file using esmodules.

I'm pretty sure this hack was used to prevent rollup from adding a node polyfill but I don't know why/if you couldn't explicitly tell rollup to not do it. Either way @threepointone hopefully has more context.

[whodoeshua]

I just try different kind of require in @testing-library/react/dist/flush-microtasks.js to find out why nodeRequire will failure in esm. My code is only transpiled by tsc.

So that mean webpack_require is the only way to make sure testing will be success? Then why don't we import a library to insure the testing is running like what we expect for?

[kentcdodds]

Actually this has nothing to do with webpack and you definitely don't need to be using webpack for this to work.

[eps1lon]

to find out why nodeRequire will failure in esm.

It will fail since you are not allowed to mix module systems in node. You either use commonJS (require) or esmodules (import).

I don't know about your use cases but usually you don't need esmodules in node since these are only useful for optimizations. I would try making it work without -r esm and rather let babel transpile it all to commonjs modules (either extra step or babel/register).

[whodoeshua]

There is an library that only transpile to esmodule and I don't want to bundler it into my library. I want to just keep import in my release file. But I will bring some issue in my testing, so I need to use esm make it work in my testing.

[just-boris]

This seems to be an issue with React itself too: facebook/react#18589

This should be fixed by this one-liner:

-enqueueTaskImpl = nodeRequire('timers').setImmediate;
+enqueueTaskImpl = nodeRequire.call(module, 'timers').setImmediate;

However, I am not sure if fixing this on React-testing-library alone would help, or the React test-utils also need to be fixed too.

[kentcdodds]

If that fixes it for us, then let's go ahead and do that (seems pretty harmless to me).

[threepointone]

yeah this looks good. I’d recommend sending a PR to react as well.

[kentcdodds]

🎉 This issue has been resolved in version 10.0.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀