-
Notifications
You must be signed in to change notification settings - Fork 69
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
chore: added a check for write finish #63
base: master
Are you sure you want to change the base?
chore: added a check for write finish #63
Conversation
See this issue: lukejacksonn#62
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
Then do a null check for
That way the same code could handle the first and subsequent calls. |
Yes, ... as usual once it started working I just committed :) - I have removed the duplicated code - inserted |
Perfect, exactly what I was imagining 💯 |
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? |
tested smaller delay of 10ms that also worked - but the delay might be too small for different hardware setups !?
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. |
Hey, just wanna warn those who are working with this code. |
See this issue: #62
I've tested this by generating a 28 kloc js file - which previously broke the build.