chore(deps): update chokidar to v4

[Fuzzyma]

This PR aims to close https://github.com/webpack/webpack-dev-server/issues/5368

In order to replace chokidars removed glob functionality, we have to use some trickery.

• glob-parent is used to get the common ancenstor dir that needs to be watched.
• picomatch is used as ignore-filter to make sure that only files/dirs are watched that match the given glob
• is-glob is used to only use glob logic when needed

In total this adds 4 direct or transitive dependencies. However, the update to chokidar v4 removes 12. So its a net positive

I also added a test to ensure that creation of nested directories is detected properly.

• [ ] This is a bugfix
• [ ] This is a feature
• [x] This is a code refactor
• [x] This is a test update
• [ ] This is a docs update
• [ ] This is a metadata update

NOTE: This PR needs to wait for https://github.com/paulmillr/chokidar/issues/1394. If that issue is not resolved, we have to make sure to not let undefined values slip through for watcher options.

I didn't add this change to this PR because it would add more changed lined and it might be not needed after all. That's why this PR is still draft

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Fuzzyma / name: Ulrich-Matthias Schäfer (184588dd1b651a3d10023063b4a5b50fd5da2a2e, 5d42bac4b309364f0b091762492bd519c798a92b, 970c35314b2cd27b9241cd5cd70a570ffe221dd6, 9492a00dcc8199015cdf293982319419b1184e82, a34faf65526e094d6cecae28f4b1f4a1ad964764, 3dd2b4b78546a8f5bcd56951fa2882eba9f8c188, b01a2c361866030d4552d50f490019aef90c5573, 7b113cf0a83443355e72bcc921d9a6c5d32f1b79, d883a1aee02e4c3425c0f63e12390db5f3138ce7, c5539b078a5d72be2d900e9cad40fe8307cd9d36, 337423440338fbeeed1e62e53f07620210248596)

[alexander-akait]

@Fuzzyma Can you rebase?

[Fuzzyma]

@alexander-akait i am still waiting for the chokidar issue to be resolved. So this isn't passing tests atm anyway.

That said, surely can rebase :)

[Fuzzyma]

@alexander-akait I saw that when I run the tests a lot of them fail - unrelated to my changes. What is that about?

[Fuzzyma]

@alexander-akait so there are 2 remaining issues:

• chokidar doesnt allow undefined as options anymore and crashes (even tho the types allow it... I raised an issue but they dont really wanna change it?). So we have to clean the options before we pass them to chokidar or wait for chokidar to come to the conclusion that its useful to follow their own types
• I dont know how to create tests for "ignored" files. How do you detect that nothing should happen? Maybe you can help me out here

[alexander-akait]

@Fuzzyma

chokidar doesnt allow undefined as options anymore and crashes (even tho the types allow it... I raised an issue but they dont really wanna change it?). So we have to clean the options before we pass them to chokidar or wait for chokidar to come to the conclusion that its useful to follow their own types

I.e. our logic is broken somewhere? Because I don't think someone write option: undefined, maybe you can provide an example?

I dont know how to create tests for "ignored" files. How do you detect that nothing should happen? Maybe you can help me out here

There is a test - https://github.com/webpack/webpack-dev-server/blob/master/test/e2e/watch-files.test.js#L160, just add a several cases where we have ignored from watchFiles - negative glob, nested glob (i.e. like test/**/a/**), just static value, and just when developer set directly.

In fact, this is one of the reasons why we moved away from chokidar inside webpack - it is very difficult to interact and make changes. Perhaps we should consider https://github.com/webpack/watchpack as chokidar replacer in future... we need to improve some things there.

Another solution that I am considering is the implementation of this function fundamentally in webpack (based on watchpack), so it will work not only for serve, but for watch too

[Fuzzyma]

@alexander-akait no your logic is not broken. It is usually totally acceptable to pass undefined options to functions because typescripts optional type is { foo?: string | undefined }. Chokidar breaks this because they pass the options through to the native node api and that api throws if you pass undefined.

However, yes, we do create undefineds when calling getInterval and no interval or poll is defined:

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

If they dont fix it, we have to fix it on our side.

For the tests: I already added tests there for the globs. But again: how do you test if something does not happen because it is ignored?

[alexander-akait]

If they dont fix it, we have to fix it on our side.

:+1:

Do they have a pr?

For the tests: I already added tests there for the globs. But again: how do you test if something does not happen because it is ignored?

maybe chokidar itself have a method that we can mock, well or we can try to check that the reload did not happen, but this will require timeouts, and it is slow

[Fuzzyma]

Yeah, mine: https://github.com/paulmillr/chokidar/pull/1396

Well since chokidar doesnt support globbing anymore, everything that touches globs, we have to test on our side. Or we just test how specific globs are correctly resolved into correct ignore functions or matchers? Maybe easier?

[alexander-akait]

Or we just test how specific globs are correctly resolved into correct ignore functions or matchers? Maybe easier?

Yeah, maybe

Let's first try to simplify everything in the code, maybe we can even use unit testing here and one or two integrations to understand that everything really works

[Fuzzyma]

Then I will pull out the creation of the ignore functions into own units. The we can simply test those.

[Fuzzyma]

@alexander-akait I added tests for the glob matching and also added wrapper code to support chokidars weird behavior with undefined values. It can be removed once they make up their mind. This should work now and is ready for review

[v1rtl]

Are there any blockers to this pull request?

[Fuzzyma]

Not from my side

[Fuzzyma]

I maybe found an issue with negated glob patterns. I need to figure out if everything is alright before merge

[alexander-akait]

@Fuzzyma Sorry for long delay (there were complicated personal things), what is current status? What we need to improve or wait? Will be great to landing it

[Fuzzyma]

@alexander-akait the problem i encountered was, that I was filtering out potential nested directories because i couldn't partially match glob patterns. The new idea is, to wrap chokidar and emit only the events that match the passed glob pattern. According to @43081j, this is also how it was done internally (if i remember correctly). However, it has the disadvantage that you watch directories and files you probably don't need to watch.

Because that is such a delicate topic, I created a package chokidar-glob that re-implements glob watching. It passes all old (v3) chokidar tests for globbing except for some windows tests which I cant debug on my machine. However, this is achieved by utilizing the approach above (=ignoring events and overwatching). In order to improve on this situation I would need to implement partial glob matching again and I didn't come across an easy solution for this yet.

According to james, even with this approach it should perform good enough.

The situation is not ideal. I don't feel comfortable recommending chokidar-glob yet (at minimum I have to fix the windows tests). I am open to suggestions on how to approach this topic further.

[alexander-akait]

Honestly, one of the reasons why we stopped using chokidar inside webpack is a lack of support and quite strange design decisions, of course this is just my opinion, but write globs for such tasks this is a basic thing, the support of which does not even make sense to argue about, it is like we don't have it in terminal, and now we have to do such strange things, but unfortunately we can't influence this, but I still think to migrate on watchpack in the next major release, because good globbing support was the only reason why they used chokidar

[alexander-akait]

@Fuzzyma I see your problem... A possible solution is to also abandon the use of globes in favor of ignore, using RegExp's or picomatch is not difficult, yes, it looks like a small regression in usability

[Fuzzyma]

The problem with ignore in chokidar is, that it will ignore a directory and also all subdirectories. If you would rather go with watchpak, thats obviously a solution, too. However, how far away is a new major?

[alexander-akait]

how far away is a new major?

We don't have roadmap for it current...

If you would rather go with watchpak, thats obviously a solution, too.

I am if we don't find solution for globing I prefer to use watchpach (less deps), because it does the same

[Fuzzyma]

@alexander-akait as far as I can see watchpak also doesn't support globs for watched files. Only the ignore prop supports globs (at least that's what I grasped from their docs and tests)

[43081j]

Just a couple of comments:

Chokidar does have support. I joined as a maintainer last year and have been very active maintaining it, responding to issues, etc

Shipping glob support in a FS watcher doesn't make much sense. It makes more sense to be able to combine an fs watcher with a glob library (also what fdir does). We did the right thing removing glob support from chokidar but it may make sense to have a higher level library soon that combines them with it

Chokidar exists to deal with the various inconsistencies between file systems and to add a higher level interface to FS watch. It doesn't exist to provide you globs

If you need any help, here I am.

It's possible the pr for undefined options will get merged at some point, but you should really solve that here too. It seems odd to be passing that

[alexander-akait]

@43081j

Shipping glob support in a FS watcher doesn't make much sense. It makes more sense to be able to combine an fs watcher with a glob library (also what fdir does). We did the right thing removing glob support from chokidar but it may make sense to have a higher level library soon that combines them with it

It looks strange because if you look at most libraries/tools/etc that use chokidar you will find that almost all uses globs, yes, it is will great to have a library that will use chokidar under the hood and support glos and perhaps make it easier for you to support, but at the present time we do not have it and this functionality is cut, we as users of this tool are forced to either remove it too or implement the logic through strange approaches and hacks, I do not think that this is a good solution

The problem is that no matter how many versions of chokidar we used in webpack, we encountered similar problems with almost every major version, although at some point we were almost one of the most popular tools that used chokidar. And I'm not just talking about the dev server now, webpack@1, webpack@2, webpack@3 and other tools where we used chokidar.

Perhaps this is how you see the approach to supporting the tool, and I have no right to argue here, because this is your vision, but speaking as a user and as a developer who uses the tool, such an approach unfortunately does not suit me, you have the right to disagree again.

Some time ago I already wrote about this, unfortunately I can't find where, yes this is my view on this problem, because every time an update is painful, as a minimum for software it is normal to first declare something obsolete/deprecated, and then delete it (i.e. one major release with deprecation and then remove it), which gives time to prepare - find other approaches to the solution, or find another tool, or try to take part of the code and logic into your code base.

It's possible the pr for undefined options will get merged at some point, but you should really solve that here too. It seems odd to be passing that

We can't control developers options here and making extra operation to remove undefined is very strange, I don't see this in other libraries at all, i.e. 'undefined' === no value and use defaults

[43081j]

I'm not saying we won't merge the change to chokidar to deal with undefined options, just that it is worth looking on your end too at why this is a thing. From what I understood, it wasn't the user passing undefined, it was webpack. But I may have misunderstood

Removing globs was a big change. But I do believed it is the right thing to do, since chokidar should focus on being a file watcher rather than a glob library.

I'm not sure yet if we should have a library which joins the two or we should let you pass one in. One or the other should exist to make this easier in future though I think

For now, it really shouldn't be so difficult to bring your own. So I'd like to understand what the difficulties are a bit better. That will help me with the end solution too

[Fuzzyma]

@43081j passing one in sounds interesting. But then again: maybe just give me more control. e.g. an event filter to effectively ignore events but not ignore watching. We talked about this at some point that chokidars kinda needs 2 types of ignore. Then we could easily just filter the events by a glob. Right now, I need a whole chokidar wrapper for that.

I think until this is figured out, a library that combines chokidar and glob makes sense. Also useful for people that don't want to wire this up themselves.

To the undefined problem: Yes, the one undefined came from something internally. But I think webpack-dev-server allows the user to pass arbitrary watcher options. So if they pass undefined, it will break even if the one occurrence in webpack-dev-server is fixed.

[43081j]

To the undefined problem: Yes, the one undefined came from something internally. But I think webpack-dev-server allows the user to pass arbitrary watcher options. So if they pass undefined, it will break even if the one occurrence in webpack-dev-server is fixed

I understand but one doesn't justify the other. We should still fix the code here to stop passing undefined. Even if a user can forcefully do it themselves

I'll have a deeper think about how best to solve this glob stuff and get back to you soon

[Fuzzyma]

I understand but one doesn't justify the other. We should still fix the code here to stop passing undefined. Even if a user can forcefully do it themselves

Oh yeah definitely!

[kevinmpowell]

Just chiming in as someone following this thread. Our org would benefit from this being merged (it would clear some security vulnerability flags we're having to ignore currently).

I see it's still a "draft" - any timeline on when this might get merged?

[alexander-akait]

@kevinmpowell we can't merge it because we will lost glob support in watch files (it is very popular usage) and there are not alternative solutions/API to make it works again