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

Fixes the dcdr-js client's file watcher #4

Merged
merged 2 commits into from
Nov 2, 2018

Conversation

stateoflux
Copy link
Contributor

I discovered that the dcdr-js client was not responding to any changes made to the underlying decider.json file. The client was using node's fs.watch to implement its file watching capability, but turns out fs.watch is notoriously buggy. I've replaced fs.watch with the chokidar library (note this library used by many of the js build systems out there), which is a wrapper around the fs.watch but fixes its shortcomings. I've also written a unit test to cover file watching functionality.

… since its a more robust file watcher. adds unit test to cover file watching
Copy link
Contributor

@bshackelford bshackelford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thank you.

Copy link
Contributor

@promiseofcake promiseofcake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Only question would be whether or not we want to commit package-lock.json since we aren't currently targeting builds > Node 6 for this repo. If we have use-cases for newer versions it might make sense to add them to build build matrix.

@stateoflux
Copy link
Contributor Author

@promiseofcake I've removed the package-lock.json

@stateoflux stateoflux merged commit c913434 into master Nov 2, 2018
@promiseofcake promiseofcake deleted the wam/cs-456-fix-watcher branch November 2, 2018 20:55
@promiseofcake
Copy link
Contributor

Thanks @stateoflux, I think if we are using this in newer node deployments we can also increment the build matrix / deprecate old builds if desired.

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.

3 participants