Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't detect change in nested files (on Linux) #36

Closed
chiefjester opened this issue Jan 20, 2020 · 23 comments · Fixed by #39
Closed

Doesn't detect change in nested files (on Linux) #36

chiefjester opened this issue Jan 20, 2020 · 23 comments · Fixed by #39

Comments

@chiefjester
Copy link

Hey, Luke 👋🏻

So I was trying snowpack out, and the docs brought me here. Their doc highly recommends servor. 😉 I've watched you in YT on pushing the web forward on a bundle-less future. I'm excited that it's becoming more mainstream.

So I'm using this in Ubuntu via WSL / Windows 10; while it reloads fine when I change the index.html, it doesn't reload if I change the JS file.

Here's a screencast I made to document the error.

@chiefjester
Copy link
Author

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.
Alt text

@lukejacksonn
Copy link
Owner

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!

chiefjester added a commit to chiefjester/servor that referenced this issue Jan 22, 2020
- fixes lukejacksonn#36
- fs.watch recursive option is only supported on macOS and Windows (see https://nodejs.org/docs/latest/api/fs.html#fs_caveats)
- add node-watch as a thin wrapper for fs.watch
- node-watch author claims it's much faster and more memory efficient than fs.watch
@chiefjester
Copy link
Author

chiefjester commented Jan 22, 2020

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
Copy link
Owner

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?

@chiefjester
Copy link
Author

chiefjester commented Jan 23, 2020

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
Copy link
Owner

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!

@chiefjester
Copy link
Author

@lukejacksonn no problem, take your time ✌🏻

@osdevisnot
Copy link
Contributor

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
Copy link
Contributor

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

@chiefjester
Copy link
Author

@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
Copy link
Owner

lukejacksonn commented Jan 28, 2020

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!

@chiefjester
Copy link
Author

@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.

@chiefjester
Copy link
Author

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.

Alt text

@lukejacksonn lukejacksonn changed the title doesn't detect change in js file Doesn't detect change in nested files (on Linux) Jan 28, 2020
@lukejacksonn
Copy link
Owner

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
Copy link

nakleiderer commented Feb 1, 2020

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.

@chiefjester
Copy link
Author

@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
Copy link

@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 :)

@chiefjester
Copy link
Author

@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.

@lukejacksonn
Copy link
Owner

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?

@chiefjester
Copy link
Author

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

image

@chiefjester
Copy link
Author

chiefjester commented Feb 9, 2020

@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:
image

@lukejacksonn
Copy link
Owner

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.

@chiefjester
Copy link
Author

@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 🙌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants