Chokidar chokes on options that are explicitely set to "undefined"

[Fuzzyma]

Describe the bug

Chokidar v4 doesnt clean up the passed options. Setting an option to explicit undefined will make node throw.

Versions (please complete the following information):

• Chokidar version: 4.0.1
• Node version: 22
• OS version: Ubuntu

To Reproduce:

Steps to reproduce the behavior. Include filename and chokidar config.

Ideally prove a problem by isolating and making it reproducible with a very short sample program, which you could paste here:

const chokidar = require('chokidar');
const fs = require('fs');
// Passing undefined will crash node
chokidar.watch('test*', { interval: undefined }).on('all', (event, path) => {
  console.log(event, path);
});
fs.writeFileSync('test.txt', 'testing 1');

Expected behavior
It should filter out undefined values so that chokidars defaulst are used instead (which doesnt happen because the explicit undefined overwrites the default when spreading):

chokidar/src/index.ts

Lines 361 to 377 in 5e6daaa

	const opts: FSWInstanceOptions = {
	// Defaults
	persistent: true,
	ignoreInitial: false,
	ignorePermissionErrors: false,
	interval: 100,
	binaryInterval: 300,
	followSymlinks: true,
	usePolling: false,
	// useAsync: false,
	atomic: true, // NOTE: overwritten later (depends on usePolling)
	..._opts,
	// Change format
	ignored: _opts.ignored ? arrify(_opts.ignored) : arrify([]),
	awaitWriteFinish:
	awf === true ? DEF_AWF : typeof awf === 'object' ? { ...DEF_AWF, ...awf } : false,
	};

Additional context
I was in the process of testing the feasability to update to v4 in webpack/webpack-dev-server#5368.
However, all watch-tests fail with the error TypeError: The "interval" argument must be of type number. Received undefined.

So either chokidar was cleaning up options in v3 or something else than Nodes native watcher was used.
In any case it is an unexpected breaking change.
I am aware that v4 was a major version bump but this seems to be a minor thing that could be adapted easily to make upgrading to v4 even easier.

Changing the above code to this would solve the problem:

        const opts = {
            ..._opts,
            // Defaults
            persistent: _opts.persistent ?? true,
            ignoreInitial: _opts.ignoreInitial ?? false,
            ignorePermissionErrors: _opts.ignorePermissionErrors ?? false,
            interval: _opts.interval ?? 100,
            binaryInterval: _opts.binaryInterval ?? 300,
            followSymlinks: _opts.followSymlinks ?? true,
            usePolling: _opts.usePolling ?? false,
            // useAsync: _opts.useAsync ?? false,
            atomic: _opts.atomic ?? true, // NOTE: overwritten later (depends on usePolling)
            
            // Change format
            ignored: _opts.ignored ? arrify(_opts.ignored) : arrify([]),
            awaitWriteFinish: _opts.awaitWriteFinish ?? (awf === true ? DEF_AWF : typeof awf === 'object' ? { ...DEF_AWF, ...awf } : false),
        };

[cndreisbach]

I am experiencing this same issue, preventing me from upgrading to Chokidar v4.

[paulmillr]

Why would you set EXPLICIT option to undefined? Simply don't pass it.

[Fuzzyma]

Some consumer packages seem to not properly clean up the options before passing. So for them, if they see their tests fail after updating, they might just revert back.
It is an easy fix to sort them out on your side. Therefore I think it's the right call to increase adoption

[paulmillr]

It’s breaking release. We’ve removed legacy code.

Consumers can easily do this on their own.

[43081j]

@Fuzzyma in your particular error (interval is undefined), where did it originate from?

Just curious of if we're being too strict at the point we do whatever type check it is that's happening

[Fuzzyma]

@paulmillr I know that and I am also in no way demanding that you change the code.
I am just saying it is one more thing that makes an otherwise easy upgrade unfeasible for a lot of people. And I think I am not alone when I say that I want v4 to get adopted as fast as possible.
The codechanges required are also minimal and the behavior kinda in line what I would expect as a user.

@43081j I came across this when updating webpack/webpack-dev-server#5374. Ofc I could easily fix this in their codebase but I wanted to make minimal changes to make review easier and merge more likely.

Chokidar v4 is in most cases a drop in replacement if the consumer didnt use globs. Not allowing undefined as options (which worked before) makes it less that

// EDIT: It is also way easier to pass in an option with a ternary when allowing undefined:

// undefined allowed
{
  someOption: someCondition ? true : undefined
}

// undefined not allowed
{
  ...(someCondition ? { someOption: true } : undefined)
}

// EDIT 2: I would also say that the types reflect what I am saying. Because Partial<{ someOption: boolean }> will end up as { someOption?: boolean | undefined }. I probably should have lead with this argument because its public api:

chokidar/src/index.ts

Lines 49 to 54 in 5e6daaa

	export type ChokidarOptions = Partial<
	BasicOpts & {
	ignored: Matcher | Matcher[];
	awaitWriteFinish: boolean | Partial<AWF>;
	}
	>;

[43081j]

i understand all of that, im just trying to find out where the error came from in the source

what line exactly in chokidar threw?

you might just be covering up some overly strict type check we're doing, by allowing undefined at the top level

[Fuzzyma]

They do their own config parsing and seem to introduce undefined there. Must be somewhere here: https://github.com/webpack/webpack-dev-server/blob/09f6f8eb46abce836acbc1b8c892e348106c924e/lib/Server.js#L875

In particular for interval they check if its not undefined. But if none of the conditions match (because interval and poll wasnt passed), it returns nothing = undefined:

https://github.com/webpack/webpack-dev-server/blob/09f6f8eb46abce836acbc1b8c892e348106c924e/lib/Server.js#L899-L911

EDIT: I just realized you were talking about chokidar and not webpack-dev-server.
Chokdidar throws because it passes the undefined though to the native node api. And node throws when it sees undefined.
So either fs.watchFile or fs.watch

Stacktrace:

 TypeError: The "interval" argument must be of type number. Received undefined

      at setFsWatchFileListener (node_modules/chokidar/src/handler.ts:332:25)
      at NodeFsHandler._watchWithNodeFs (node_modules/chokidar/src/handler.ts:393:16)
      at NodeFsHandler._handleDir (node_modules/chokidar/src/handler.ts:654:21)
      at NodeFsHandler._addToNodeFs (node_modules/chokidar/src/handler.ts:707:29)
      at node_modules/chokidar/src/index.ts:466:21
          at async Promise.all (index 0)

chokidar/src/handler.ts

Line 333 in 69c115a

watcher: watchFile(fullPath, options, (curr, prev) => {

[43081j]

makes sense

so we have a few options:

1. leave it as is, the burden is on the consumer and means some upgrades are non-trivial
2. individually default all options rather than using object spread (more code, but makes upgrade trivial in these cases)
3. handle the interval case by itself

@paulmillr if we did option 2, it goes from this:

const opts = {interval: DEFAULT_INTERVAL, ...userOptions};

to this:

const opts = {
  interval: userOptions.interval ?? DEFAULT_INTERVAL
}

for every option

it is true this will make upgrading easier, at the cost of us introducing more code. what are your thoughts?

i think my opinion is that it won't be much code for us to add, so maybe its worth it to help with people upgrading

[Fuzzyma]

I would be happy to provide a PR so you can weight the actual changes required.

I also want to reiterate, that passing undefined to any option is currently backed by the typings that are public to the consumer since optional parameters allow undefined by definition. So you probably will end up getting reports for the same issue again and again

[43081j]

that's true (unless exactOptionalPropertyTypes is set i guess but that seems like a rarity currently)

if its easy enough, a PR would help and we can review 👍

[paulmillr]

Main upgrade points is lack of globs (harder filtering), lack of native fsevents on macos (resource limits).

i would suggest to add an utility function in your webpack code that would clean the options before passing them to chokidar. Once some time passes and you’re fine with the upgrade (some users aren’t), we can consider it. I don’t want to add unnecessary code for users who don’t upgrade in the end.

[Fuzzyma]

@paulmillr well technically I even removed a line. Its really not much code (#1396)

[benmccann]

I'll add my 2 cents that I think this would be nice to handle on the chokidar side. I generally expect undefined and missing properties be handled in the same manner. It's just nicer as a consumer of the library to have less to worry about