TypeError unref of undefined when running proper-lockfile inside of react-scripts test

[mboperator]

Background

I'm mounting ipfs, which has a dependency on ipfs-repo inside of a react app generated via create-react-app. Running a very basic render test results in the following exception.

Problematic Command

$ NODE_PATH=src/ CI=true react-scripts test && yarn run lint

Exception

/Users/marcusbernales/Workspace/odyssey/node_modules/proper-lockfile/lib/lockfile.js:180
    if (lock.updateTimeout.unref) {
                           ^

TypeError: Cannot read property 'unref' of undefined
    at updateLock (/Users/marcusbernales/Workspace/odyssey/node_modules/proper-lockfile/lib/lockfile.js:180:28)
    at /Users/marcusbernales/Workspace/odyssey/node_modules/proper-lockfile/lib/lockfile.js:252:17
    at /Users/marcusbernales/Workspace/odyssey/node_modules/proper-lockfile/lib/lockfile.js:42:17
    at /Users/marcusbernales/Workspace/odyssey/node_modules/proper-lockfile/lib/mtime-precision.js:39:13
    at callback (/Users/marcusbernales/Workspace/odyssey/node_modules/graceful-fs/polyfills.js:295:20)
    at callback (/Users/marcusbernales/Workspace/odyssey/node_modules/graceful-fs/polyfills.js:295:20)
    at FSReqCallback.oncomplete (fs.js:170:5)

Potential Solution

Modifying lockfile.js:180 to check for the existence of lock.updateTimeout first solves the runtime error. Would love to discuss whether that's the right way to go about it.

[satazor]

Looking at the code, the error you reported shouldn’t be happening as setTimeout should always return something. In case of the browser, it returns a integer, while in node it returns a time ref. It seems to me that your test environment is somehow mocking setTimeout in a bad way and returning nullish, causing the issue.

Are you using any lib to mock timers?

[mboperator]

Thanks for getting back @satazor
Looks like jest is doing something funky with setTimeout.

I've attemped using jest.useFakeTimers() but it looks like the mock timers don't solve the issue.

I was able to confirm that creating a manual mock of setTimeout which returns an empty object fixes the unref of undefined issue.

It'll take further digging to figure out exactly what jest is doing that is causing issues with setTimeout

[jessebmiller]

I'm doing the same, and seeing the same error.

The PR that @mboperator submitted (#91) mitigates the issue in my case.

Given the fact that it's necessary to be defensive here for cases where setTimeout returns something specifically different (an integer), it seems reasonable to me to be generally defensive against cases where it might return something else (like in this case undefined).

The current implementation is defensive against everything that happens to be truthy but doesn't implement unref. Why not go all the way and defend against nullish values as well?

[satazor]

I understand that the overall issue caused a lot of frustration. If the check was in place, you wouldn’t experience this issue at all, which at first might be a good thing. But if you think about it, it would be hiding a flaw from somewhere else that would eventually bite you in the future.

The spec says that setTimeout must return a handle that can be passed to clearTimeout, see https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers

Since this is a bug with the environment you are running tests and not this lib, it seems unreasonable to implement a fix/check here.

[jessebmiller]

Forgive me, I'm pretty new to reading specs like this, but it looks to me like undefined can be passed to clearTimeout.

If it really is against the spec to return undefined from setTimeout I'm 100% with you, however I don't see in the spec you linked that this is quite the case.

"The clearTimeout() and clearInterval() methods must clear the entry identified as handle from the list of active timers of the WindowOrWorkerGlobalScope object on which the method was invoked, if any, where handle is the argument passed to the method. (If handle does not identify an entry in the list of active timers of the WindowOrWorkerGlobalScope object on which the method was invoked, the method does nothing.)"
-https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers

clearTimeout seems to be designed to accept arguments that don't identify handles and that this would include nullish values like undefined which to me seems reasonable for situations like unit testing.

[jessebmiller]

Ah just read a little more and I see where it is required to be an integer.

"If previous handle was provided, let handle be previous handle; otherwise, let handle be a user-agent-defined integer that is greater than zero that will identify the timeout to be set by this call in the list of active timers."

I'm happy to go make this argument with Jest

[hugomrdias]

This is an old issue i'm going to close this! Feel free to reopen if it needs more discussion.

Thank you.