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

chore: added a check for write finish #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

David-Klemenc
Copy link

See this issue: #62

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

  • the delay of 150 is ... random ... could be passed as a parameter?
  • I've tested this on mac-os, have not tested on win (and did not touch the linux code)

@lukejacksonn
Copy link
Owner

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

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

Perfect, exactly what I was imagining 💯

@lukejacksonn
Copy link
Owner

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 !?
@David-Klemenc
Copy link
Author

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

Ledzz commented Feb 16, 2023

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants