search

lukejacksonn / servor

Dependency free file server for single page app development
MIT License
1.04k stars 70 forks source link

chore: added a check for write finish #63

Open David-Klemenc opened 3 years ago

David-Klemenc commented 3 years ago

See this issue: https://github.com/lukejacksonn/servor/issues/62

I've tested this by generating a 28 kloc js file - which previously broke the build.

lukejacksonn commented 3 years ago

Thanks for this David! Know that I have looked at it but haven't had time to pull it down and review it properly yet. Will try get around to it this weekend if not before.

My only comment on first glance is that there is some duplicate code in the fs watch callback and in the awaitWriteFinish function. Do you think its possible to set it up so that we can call the recursive function like:

awaitWriteFinish(fileChanged, null, cb)

Then do a null check for prev as well as (and before) checking the time? 

if (prev && stat.mtimeNs === prev.mtimeNs)

That way the same code could handle the first and subsequent calls.

David-Klemenc commented 3 years ago

Yes, ... as usual once it started working I just committed :) - I have removed the duplicated code - inserted {} instead of null - so the first comparison is undefined === <some_number>

lukejacksonn commented 3 years ago

Perfect, exactly what I was imagining 💯

lukejacksonn commented 3 years ago

Ok I just pulled this down and looked at it.. my only question now is around th 150ms. Like you say, it probably should be configurable really but if we were to choose a constant, then could we get away with something lower, like unperceivably low, something in the order of 10s of milliseconds.

Perhaps 50ms. Can you foresee any issue with lowing it by 3x?

David-Klemenc commented 3 years ago

I have tested with a timeout of 0 and it does not work, then I tried 50 and it worked and 10 and it also worked. I'm afraid this setting might be hardware dependent.

For testing I generated a file that is 28k lines of code long - (a js file generated by the elm compiler) - it is long enough to trigger the file change and reload the browser window before writing to it is done.

Ledzz commented 1 year ago

Hey, just wanna warn those who are working with this code. This particular line may cause script to fail, because the filename can be null and it isn't handled.