Doesn't detect change in nested files (on Linux) about servor

[lukejacksonn]

Ok great! Now, I would like to try resolve that console error for you too.. I'm wondering if it has something to do with your finding here:

I tried to console.log inside the loop of fs.watch, I get 9 "changed" logs when I edit one file.

I'm going to look at how node-watch smooth that signal out. We know its doing too much work and thats never good. Hopefully a debounce or something fixes the issue.

[nakleiderer]

I believe I'm also running into this issue. I believe using chokidar instead of node-watch will smooth over duplicate events and operating system differences.

[thisguychris]

@nakleiderer chokidar has issues on its own too.

It was even dropped by Sapper (Svelte's NextJS) 2 years ago and is now using cheap-watch.

And I think the author wanted something internal? Maybe because of filesize? Chokidar is huge compared to the alternatives I have suggested. 21.8kb (chokidar) vs 2.6kb (node watch) or 970b (cheap-watch).

Also, servor itself is lean, around 9.7kb. I don't think putting a dependency which is twice the size of the main project makes sense.

[nakleiderer]

@thisguychris Fair points all around. I've mainly used chokidar in environments where dependency size is largely irrelevant, so I wasn't familiar with the size. Thanks for the insight!

Is it a goal of this project to be light-weight on dependencies? I see that mentioned as a feature in the docs, but I'm curious if that is intended to be a defining trait of this project or a reflection of its current state. Is it a goal of this project to be used in deployed environments or as a development tool? If it's a development tool, then dependency weight is less important in this project than it is for Sapper, which is run in deployed environments. I agree that chokidar's size is disproportionate to the current size of this project, though it appears to be a more mature project than some of the alternatives with a large following. Popularity isn't everything though.

Obviously, I'm happy with any solution to the problem. I think a clarification on the principles/goals of this project can help the community contribute more effectively. Again, thanks for the reply :)

[thisguychris]

@nakleiderer I actually agree with you on size being less important, since this is a dev dependency.

If the file size is the problem, then I would argue that this is a dev dependency and file size is not that going to a big deal especially it's only 2.6kb

But the author seems he wanted this to be fixed at the main library level without dependency. Let's wait. The main thing here is we can't rely on Node's Native fs.watch. It's a buggy implementation. 🙊 That's the whole reason, chokidar, node-watch, cheap-watch, and other permutations exist.

[thisguychris]

Hello, 👋🏻

So I tried this in a Linux Machine, particularly Ubuntu 19. It doesn't work either. Updating JS doesn't live-reload but updating the HTML does.

[lukejacksonn]

Hey @thisguychris and welcome 😅 so I hadn't tested servor on linux (sorry).. but congratulations it seems like you have found a bug.

The good news is that is is probably fixable, the bad news is that I'm not 100% sure what is going wrong, at least not off the top of my head.

Could you clone the project potentially and then try doing some console logs around the live reload functionality specifically around here https://github.com/lukejacksonn/servor/blob/master/servor.js#L125 in order to see if the node file watcher is playing up or if it is the EventSource.

Thanks for the report and let me know if you are struggling to debug at all!

[thisguychris]

Hey @lukejacksonn 👋🏻

So the culprit was node's native fs.watch which doesn't support the recursive option on Linux

I've opened a PR #37 to replace it with node-watch; the usage and option are the same as fs.watch.

From testing, this is a faster implementation. I tried to console.log inside the loop of fs.watch, I get 9 "changed" logs when I edit one file. With this change, I get 1:1 mapping for changed files.

I guess related to #10. This change would reduce false positive / extra reloads made by servor via node's native fs.watch.

[lukejacksonn]

Humm.. I did suspect that might be the case! Thanks for the investigation and for the PR with the fix in it. Now come the tricky bit.. this project was built to be dependency free. I was aware of the fickleness of node fs.watch implementation but until now we have got away with it 😅

I just looked at the source of node-watch and it seems like it itself is dependency free (which is great). Perhaps that means we could try learn-by-doing and create our own utility that works recursively on linux (and maybe even do some event de-duplication to get a better change mapping).

What are you thoughts on this?

[thisguychris]

Hi @lukejacksonn 👋🏻, I've certainly hesitated at first, before adding a dependency since I did see your code was straight forward and dependency-free. But I also believe it's better to leverage.

The goal of this project is to have a straightforward live-reloadable server. It just so happens that the native fs.watch is buggy on other platforms in this case Linux. Now if we start on monkey patching native functionality, then I feel that we're moving away from that goal.

If the file size is the problem, then I would argue that this is a dev dependency and file size is not that going to a big deal especially it's only 2.6kb

There is another leaner package than this like cheap-watch, which is also used by svelte/sapper . We can use that instead if you like? This brings down the dependency to 970b, less than 1kb

I would also add, that if we were to write an implementation of a file watcher, then that should be a library in itself or a separate file. Which kinda feels like a dependency on itself even if we package it inside of servor. That way should Node fix it's fs.watch, then we could just swap that library with native calls again. I hope that makes sense. 😅

Of course, I'll leave up the decision for you. ✌🏻

[lukejacksonn]

Ok cool. Thanks for taking the time to conduct this research and explore options! I will take a closer look at what is required to make it work cross OS (by examining node-watch and cheap-watch to see what/how they do it).

Will get back to you once I have decided on what action to take.. hopefully not too long from now!

[thisguychris]

@lukejacksonn no problem, take your time ✌🏻

[osdevisnot]

I'm not necessarily advocating this approach, but I've had a decent success bundling in the dependencies for klap https://github.com/osdevisnot/klap/blob/master/package.json#L18

This probably gives you best of both worlds, reuse the code from other packages (+ keep up with updates), and users don't have to install any dependencies.

[osdevisnot]

BTW, you can exercise little more control over dependencies by monkey patching the dep code in trivial cases. For example, here is how I get servor to watch only dist directory - https://github.com/osdevisnot/klap/blob/master/patches/servor%2B3.1.0.patch

[thisguychris]

@osdevisnot Nice, thanks for the share! klap looks amazing. I've heard of ncc by zeit before, but you're right, this actually makes the bundle smaller too. It solves the problem of external dependency.

[lukejacksonn]

Hey @thisguychris I think we have fixed the recursive issue for linux here ☝️ could you please check it out for us and confirm wether or not it works, thanks!

[thisguychris]

@lukejacksonn hello 👋🏻. I just checked in Ubuntu via WSL2, it indeed fixes the recursive option for all the files 🙌🏻

I'll update this thread, once I test on a Linux box. But my guess it's going to be the same.

[thisguychris]

I'm back 🤗

So it seems that it's recursive watch is also working on Linux Ubuntu 19. With the exception of a console error, it seems it's working fine.

[lukejacksonn]

Hey @nakleiderer and @thisguychris I have been away on vacation the past week. Only just getting the chance to catch up on Github notification. To answer some of your questions:

Is it a goal of this project to be light-weight on dependencies?

Yes, it is the goal of this project to stay dependency free 🌈 and so far we have managed it! the solution to this problem has also been resolved without a dependency (see #39).

Given the application of watch here I am still unsure wether the duplicate events are an issue; even if it fires 10 change messages at a time the browser reloads on the first message and thus ignores subsequent messages. So there shouldn't be any observable adverse side effects.

After some experimentation before my vacation.. I think the error that @thisguychris was experiencing was due to the keep open connection not being terminated properly. So I have had a go at adding some code that does that.

@thisguychris can you describe a bit more precisely when the error occurs?

[thisguychris]

@lukejacksonn Sorry, I missed this. On startup I don't see any error at first. However, it happens the moment I change anything.

[thisguychris]

@lukejacksonn so I think the error comes from this line

const source = new EventSource('/livereload');

And it only happens on Firefox. It's not showing an error on Chrome/Chromium based browsers.

When I click on the error it points to this line:

[lukejacksonn]

Alright.. interesting, thanks! I'm not quite sure what it might be.. I think it has something to do with not closing the socket properly but it's hard to tell. Maybe #41 might help 🤷‍♂

Anyway. I am happy that #39 resolves the recursive watch issue on linux. Hopefully with no adverse effects on other platforms. Going to get it merged asap, along with #41.

[thisguychris]

@lukejacksonn I think we can close this now once #39 is merged. And discuss the last error in #41. Which I think is related. Thanks for the fix 🙌🏻