Revisit the watch option and chokidar dependency

[rdmurphy]

Hello!

I was digging into what was inflating the install size of nunjucks and finally traced it down to the chokidar dependency that's included in optionalDependencies. At one point (in #610) it was even discussed that nunjucks just get rid of this capability entirely — which I think is the right answer overall.

(I got on this path when I was comparing Liquid JS to nunjucks and noticed how much tinier it was for similar capability, and it has everything to do with it bypassing the work of including a watcher.)

Even now as an optional dependency it's practically a guarantee that most people are still downloading it — it's uncommon for folks to pass in the --no-optional flag and AFAICT it's not documented as an installation method for nunjucks. I'd argue it'd perhaps make more sense as a peer dependency or... just not there at all. 😄

I'd be happy to help make this a reality if there's still an appetite for it! Seems like something that could be re-created as a recipe/example of how to do it externally so people still have a path to accomplish this.

[rdmurphy]

A friendly ping on this! 👋

[fdintino]

I'm hesitant to remove it altogether, as there are various tools that rely on nunjucks' built-in watch functionality. But I don't see any harm in moving it to peerDependencies; the code is already written so that it runs fine in the absence of chokidar, provided the watch option isn't used.

[rdmurphy]

Hey @fdintino! Thanks for the response.

I poked around in other libraries that do things similar to this (primarily rollup), and I think there is kind of an issue with both optionalDependencies and peerDependencies.

As I mentioned above, optionalDependencies has the problem of always being installed with an npm install or yarn unless you explicitly tell them not to, and it's an all or nothing kind of deal. If any other library has optional dependencies, they get excluded too. Apparently the reason for this is because optionalDependencies is meant to list packages that are okay if they fail to install during the install process (like if a package won't build on Linux, but you don't care and hedge around that in your code).

peerDependencies sound like the better option, but it has its own issue — every time someone installs nunjucks the installer is going to "warn" the user in the terminal that nunjucks has an unmet peer dependency.

You can see this happen for example when you install @babel/preset-env without @babel/core.

warning " > @babel/preset-env@7.4.5" has unmet peer dependency "@babel/core@^7.0.0-0".

This isn't ideal, because it makes it sound like you're expected to install chokidar on your own for nunjucks to work in all cases, which isn't true — it only matters during a watch.

Which brings me to rollup — it has a watch functionality as well that by default does not depend on chokidar but can optionally use it. But the way it does "optionally" is by not having it appear in dependencies at all. It takes the approach of suggesting (via documentation and a thrown error) that if you wanna use chokidar you gotta install it yourself and pass the flag.

But in nunjucks case because there's already a check for it, we could just change the error to say "hey, install chokidar if you're trying to use the watch functionality". Then, folks who depend on it still have a clear path to make it work but by default it's not there.

I think no matter which path nunjucks takes, this would probably be a major version release to cushion the blow on any folks who are currently using it — a minor release could surprise someone even if it got moved to peerDependencies because it won't get installed by default anymore if they update. So in theory there's space for a bigger change (like making it optional without being a dependency)!

[MH15]

Any updates on this? Chokidar alone is almost the same size as the rest of Nunjucks and its dependencies.

[fdintino]

@MH15 ignore my comment in the other issue. You're right, I hadn't realized the optionalDependencies behavior was still causing this. As @rdmurphy explained in his very thorough comment above, it's a large enough change that it probably ought to be rolled out with a major release. The good news is that the plan for nunjucks v4 involves splitting some optional functionality into separate namespaced packages, e.g. @nunjucks/watch, which would effectively fix this bug.

[Krinkle]

Perhaps node-watch is a workable alternative?

It's a simple, well-tested, dependency-free implementation of a recursive directory watcher.

It was adopted by QUnit last year, and for the same reason (impractically large dependency graph). See also qunitjs/qunit#1342.

Let me know if you're open to this, as I'd be interested in writing a patch for it.

(Ref 11ty/eleventy#1375.)

[fdintino]

I would absolutely welcome a PR that switched from chokidar to node-watch!

[rdmurphy]

I don't have strong feelings on this, but when I originally added this issue is was right after Chokidar's 3.0.0 release which made it drastically smaller, which lessened the issue once it made it into nunjucks. The gains here now from switching to a new watcher aren't too crazy. (And I personally trust Chokidar more just because it is more battle tested.)

But I think independent of which watcher is in use there's still the challenge of nunjucks shipping dependencies you'll never need if you don't use the watch functionality. As detailed above the only solution to that is a breaking change that removes chokidar from optionalDependencies.

Another path here (which I think was added as a feature to npm since this issue was created) is to move chokidar to peerDependencies and to add peerDependenciesMeta to make it optional. Then nunjucks won't yell on install if chokidar (or node-watch) is missing, but you can rely on the current prompt to guide people toward it if they do something that requires it.

[fdintino]

I think it's fine to move chokidar to peerDependencies if it's part of a minor version bump. And you're right, chokidar@^3 looks much improved. I'm a bit bogged down at the moment with a few other tasks; would you be willing to open a PR with those changes?

[rdmurphy]

@fdintino Whoops, coming back around to this!

Would it not be considered "breaking" if a user making a minor version upgrade suddenly had to install chokidar separately? (I honestly do not know. 😶) They could pick up the minor version upgrade and their use of Nunjuck's watch starts to fail. If we feel that's not a dire concern then happy to do it!

[fdintino]

I think if we make the chokidar error message more helpful, we'd cause minimal disruption. This error would never really occur in production or a CI environment. If a developer starts a watch process and hits this error, the remedy is fairly straightforward.

[rdmurphy]

Sounds good. I’ll try and throw something together.

[MH15]

Don't mean to be rude, but any updates?

[rdmurphy]

Finally took that swing at this here: #1329