-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add support for filesystem polling #1176
Conversation
So cool @aomarks, will give this a spin on Monday. |
@aomarks I gave this a go yesterday and in my first tests, using |
*/ | ||
function readWatchConfigFromEnv(): Options['watch'] { | ||
switch (process.env['WIREIT_WATCH_STRATEGY']) { | ||
case 'event': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: I definitely prefer avoid fall-through myself. Would this be easier as an if/else statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file (and other files) uses this style of switching on state
. We're doing exhaustive checking for every state for every transition. I actually kinda like using fallthrough here, it's just the most concise way to write out all the permutations.
if (intervalStr) { | ||
const parsed = Number(intervalStr); | ||
if (Number.isNaN(parsed) || parsed <= 0) { | ||
console.error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is continuing to operate with the fallback value, should this be console.warn()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both go to stderr, but in some configurations warnings might get filtered out? I guess because it's definitely something wrong, I think it should be an error to maximize the chance of somebody seeing it.
}; | ||
} | ||
default: { | ||
console.error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
💪🏼 |
Adds
WIREIT_WATCH_STRATEGY
andWIREIT_WATCH_POLL_MS
environment variables which can be used to switch filesystem detection from event-based to polling:Currently this just sets the chokidar
usePolling
andinterval
options. It might be nice to actually bypass chokidar entirely so that this option can be used to bypass any potential issues with chokidar generally, but it's slightly complex (just re-running the script on an interval isn't great, because of scripts which aren't set up well for caching).This is published as
v0.14.9-pre.1
. Would be great to hear from @peschee if this is a useful workaround for the OOM errors they have been seeing in their project.