React-testing-library: scheduleFn is calling NormalPriority as a function rather than cb

[lerouxb]

As of https://github.com/testing-library/react-testing-library/pull/732/files we're seeing:

    TypeError: callback is not a function
        at <Jasmine>
        at scheduleFn (webpack:///node_modules/@testing-library/react/dist/@testing-library/react.esm.js:230:1 <- tests.webpack.js:27483:12)
        at scheduleCallback (webpack:///node_modules/@testing-library/react/dist/@testing-library/react.esm.js:232:1 <- tests.webpack.js:27485:46)
        at Object.then (webpack:///node_modules/@testing-library/react/dist/@testing-library/react.esm.js:245:1 <- tests.webpack.js:27498:9)

Looking at that PR it seems to have two sources of truth: isModernScheduleCallbackSupported based on the react version and NormalPriority based on the existence of Scheduler.

isModernScheduleCallbackSupported seems to be true yet Scheduler undefined.

Which causes it to call scheduleFn(NormalPriority, cb) rather than scheduleFn(cb). And scheduleFn is callback => callback().

[MatanBobi]

Hi @lerouxb, thanks for sending this one.
This was done since from version 16.8.6, the Scheduler is actually there.
What's the React version you're using?

[lerouxb]

$ grep version node_modules/react/package.json
  "version": "16.13.1"

The code is finding Scheduler on globalObj. I guess it isn't guaranteed to exist on global || window in every testing setup.

[MatanBobi]

It also tries to require it: Scheduler = nodeRequire.call(module, 'scheduler');
Can you please add your testing environment?

[lerouxb]

The code seems to be treating the existence of Scheduler as optional. I think it should consistently do it in all the lines. And if it doesn't exist, then use the single parameter form of the function.

[eps1lon]

And we probably need to require the scheduler as an optional peer dependency. Otherwise it might not be available depending on the dependency tree and package manager.

[MatanBobi]

And we probably need to require the scheduler as an optional peer dependency. Otherwise it might not be available depending on the dependency tree and package manager.

@eps1lon Isn't having react-dom add it as a dependency enough in this case?
I guess no one will implicitly install scheduler since it's a react implementation detail..

[eps1lon]

Is scheduler a peer of react-dom or react? Either way, implicit dependencies are a problem. Definitely for newer package managers such as pnpm and yarn 2.

[MatanBobi]

scheduler is an implicit dependency of react-dom: https://github.com/facebook/react/blob/master/packages/react-dom/package.json#L19

[lerouxb]

Btw, I just tried npm install scheduler and that's not enough to fix this.

[lerouxb]

The line

Scheduler = nodeRequire.call(module, 'scheduler')

Is in the same try {} catch {} as the existing enqueueTask = nodeRequire.call(module, 'timers').setImmediate line. If that line fails, then Scheduler could remain undefined as it would never try and import it.

And it looks like that's exactly what's happening: the timers import is throwing and the catch is executing. (nodeRequire is undefined)

[eps1lon]

Btw, I just tried npm install scheduler and that's not enough to fix this.

I wouldn't have expected that because the package manager still doesn't know that testing-library requires it.

[bengillies]

So I dug into this further, and here's what's happening for us:

1. Scheduler here is undefined as it's not present on window
2. module here is from webpack so has no require in it. Instead it has the following keys: children, loaded, id, exports, webpackPolyfill. This means nodeRequire is false
3. nodeRequire being false means Scheduler is never imported, triggering the catch block
4. The catch block here seems to suggest that this is intentional for browsers so I'm not sure what's going on.

The bug then happens as follows:

1. NormalPriority here is null as Scheduler never imported.
2. scheduleFn here is callback => callback() as Scheduler never imported
3. isModernScheduleCallbackSupported here is still true, though, so the following code is run: scheduleFn(NormalPriority, cb), which in turn tries to call NormalPriority() (which is undefined).

Hope this makes sense

[bengillies]

Can you please add your testing environment?

This is testing through webpack + karma + ChromeHeadless

And we probably need to require the scheduler as an optional peer dependency.

I agree this is problematic but fwiw, if I add window.Scheduler = require('scheduler'); to our test setup script (that imports a whole bunch of polyfills), then everything works correctly so scheduler seems to be importing just fine without changing anything else.

Adding extra globals to window is obviously not a great solution though.

[MatanBobi]

Thanks @bengillies, that really helpful.
In that case, there's probably a Scheduler but we just fail in fetching it.
This means that defaulting to callback => callback() won't work for us in react-dom@next since React still use the Scheduler to schedule a callback.
We need to figure out a different way to pull the scheduler module.
@kentcdodds, @eps1lon any idea for a different way to require the scheduler react-dom is using?

[bengillies]

In that case, there's probably a Scheduler but we just fail in fetching it.

This sounds right to me.

I'm not really an expert on how bundlers are importing things, but it seems to me like

  // read require off the module object to get around the bundlers.
  // we don't want them to detect a require and bundle a Node polyfill.

isn't really relevant to the Scheduler as it's being imported and used directly and not polyfilled from node. If that's the case, is it just a case of importing it directly at the top of the file along with React and satisfies?

[MatanBobi]

Well, the problem is that we want the specific scheduler that your react-dom depends on. Importing one of our own will make us too coupled to a specific react-dom version.

[bengillies]

Well, the problem is that we want the specific scheduler that your react-dom depends on.

Thinking about this, I don't really understand what's different between importing Scheduler at the top of the file and the current approach of taking whatever window.Scheduler is and falling back to requireing it further down?

Importing one of our own will make us too coupled to a specific react-dom version.

Is it not already implicitly coupled to the version react-dom uses?

I guess the ideal situation would be for react-dom to expose its scheduler somewhere so that you can hook into it right? I guess that's perhaps not realistic.

[MatanBobi]

Thinking about this, I don't really understand what's different between importing Scheduler at the top of the file and the current approach of taking whatever window.Scheduler is and falling back to requireing it further down?

importing at the top of the file if there's no scheduler will yield an exception IMO..
Also, importing from a package you're not installing isn't really a good practice since we can't count of it being there, that's why we added the fallbacks..

Is it not already implicitly coupled to the version react-dom uses?

If you use [email protected] it uses a specific version of the scheduler, other version of react-dom uses other version.
The scheduler is a dependency of react-dom but if we install the scheduler we might not use the specific version that your react-dom is using.
Since we're supporting multiple version of React, we can't decide on a specific scheduler version we need to install.

[bengillies]

I can't speak to other build systems, particularly as you're mixing ES style import with node style require, but at least for us, the following two approaches both seem to work:

Approach 1:

1. Remove the call to require Scheduler from the existing try/catch block (the call to import timers will fail anyway)
2. Add in a new try/catch block just above the current one with the following:

try {
  Scheduler = Scheduler || require('scheduler');
} catch(_err) {
  isModernScheduleCallbackSupported = false;
}

or Approach 2:

Add the following line into the catch block that already exists:

  isModernScheduleCallbackSupported = false;

This then ensures that if the call to import Scheduler fails, the rest of the code doesn't assume it succeeded.

[kentcdodds]

Let's go with approach 2.

[MatanBobi]

@kentcdodds, @bengillies
The problem I see with both of the approaches is that it's a false negative..
The scheduler is there, we just weren't able to require it, meaning we will try to use the callback => callback() approach and not to schedule the callback as we're supposed to.
If I understood the scenario here correctly, this will fail us in react@next.

[kentcdodds]

I'm unsure why it is we're unable to require the scheduler if it's there

[kentcdodds]

Oh wait. I understand. I actually thought about this when I was helping work on this and this:

I agree this is problematic but fwiw, if I add window.Scheduler = require('scheduler');

was the solution I thought would work best. It should be documented. No other changes necessary. This is an edge case and I can't think of a better way to accomplish this.

[kentcdodds]

Oh, that said I think we should also make sure this doesn't throw an error like it is if someone forgets to do that. So option 2 is probably a good idea as well

[kentcdodds]

Not actually scheduling a callback with the real scheduler is only sometimes problematic, so we should be ok most of the time.

[MatanBobi]

Oh wait. I understand. I actually thought about this when I was helping work on this and this:

I agree this is problematic but fwiw, if I add window.Scheduler = require('scheduler');

was the solution I thought would work best. It should be documented. No other changes necessary. This is an edge case and I can't think of a better way to accomplish this.

@kentcdodds I can document this one and also create the fallback with option 2.
Do you think we should write a warning if it's not being added in the case we think it should be? (isModernScheduleCallbackSupported === true)
Also, where do you think we should document this?

[bengillies]

was the solution I thought would work best. It should be documented. No other changes necessary.

Oh, that said I think we should also make sure this doesn't throw an error like it is if someone forgets to do that. So option 2 is probably a good idea as well

This seems like a fair approach to me. Thanks for your help.

[eps1lon]

I'm not following why a scheduled callback flushes microtasks. Can we go back a bit and explain what we're trying to accomplish with flush-microtasks.js? It seems to me that scheduling a callback using another package is not only flushing microtasks but schedules a task with a normal priority. That a task with normal priority (defined by a scheduler package) is a microtask in javascripts event loop is (to me) an implementation detail of the scheduler package.

If we know what problem we're trying to solve we can consult with React core members. I don't think this current approach is something they want to be used by 3rd party libraries. Especially because it mixes so many (old, new, non-standard) module systems.

[kentcdodds]

The biggest problem is when using fake timers. It's really hard to explain, and I'm feeling really tired right now so I don't think I can properly explain it. but if we don't do things this way then we could end up with a situation in some cases where a scheduled to call back hangs preventing future callbacks from being run. I spent two days on this problem. I agree that it's an implementation detail, but this is why libraries like ours exist. So that the users of the library don't have to worry about the intricacies and weirdness that we do.

[eps1lon]

But this approach of doing it on our own and not adding tests (in any form) already failed for react@next. It's clear that the current approach won't scale across react versions. At some point we need to take a step back, re-evaluate the approach and make a concentrated effort to fix it. Right now it's throwing scheduling apporaches at the problem until something sticks.

I agree that it's an implementation detail, but this is why libraries like ours exist.

Why do we need to? So far react doesn't have any documentation how to use the scheduler package or how React uses it specifically. It may be possible that they don't intend for us to reach into these details. If they understand our issue they may provide a first-class API for it without us having to maintain a function for multiple versions of react.

[spectras]

I just ran into what looks like the exact same issue.
React version: 16.13.1.
Running with Karma 4.4.1 in ChromeHeadless and Chrome. Rollup.js as bundler.

  TypeError: callback is not a function
      at scheduleFn (node_modules/@testing-library/react/dist/@testing-library/react.esm.js:230:12 <- tests/index.js:47762:13)
      at scheduleCallback (node_modules/@testing-library/react/dist/@testing-library/react.esm.js:232:46 <- tests/index.js:47764:47)
      at Object.then (node_modules/@testing-library/react/dist/@testing-library/react.esm.js:245:9 <- tests/index.js:47777:10)

This exception happens immediately after the first test case. Apparently, the error happens in the afterEach hook.

Relevant code in react.esm.js is:

function scheduleCallback(cb) {
  var NormalPriority = Scheduler ? Scheduler.NormalPriority || Scheduler.unstable_NormalPriority : null;
  var scheduleFn = Scheduler ? Scheduler.scheduleCallback || Scheduler.unstable_scheduleCallback : function (callback) {
    return callback();   //<============ exception on this line
  };
  return isModernScheduleCallbackSupported ? scheduleFn(NormalPriority, cb) : scheduleFn(cb);
}

Using break on exceptions to get more info, I get:

• NormalPriority: null
• isModernScheduleCallbackSupported: true
• Scheduler: undefined
• callback: null

→ So this matches perfectly with OP's.

By bisecting versions and commits, I can tell that:

• It worked fine in v10.4.3
• It fails in v10.4.4
• It seems commit https://github.com/testing-library/react-testing-library/commit/604d3e96364175974fcb2d12fc7fa02b1c50d2e6 created the issue.

By reading the discussion, it seams I am not bringing much new. Confirmation maybe, and another setup that exhibits the same behavior.

[stanlemon]

I ran into this issue as well with a very basic test using jest.

That test is on GitHub if you want to try locally:
https://github.com/stanlemon/example-app/blob/master/src/App.test.tsx#L10

Note that to get past the immediate problem I simply did:

jest.useFakeTimers();

[kentcdodds]

This should be published in a few minutes.

[kentcdodds]

:tada: This issue has been resolved in version 10.4.5 :tada:

The release is available on:

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

Your semantic-release bot :package::rocket:

[spectras]

Confirming 10.4.5 fixes it here too.
Good job, and thanks for your excellent responsiveness!