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?