Skip to content

fix: remove watcher.close and use watcher.once#34

Merged
timneutkens merged 1 commit intovercel:masterfrom
tungv:master
Sep 12, 2017
Merged

fix: remove watcher.close and use watcher.once#34
timneutkens merged 1 commit intovercel:masterfrom
tungv:master

Conversation

@tungv
Copy link
Copy Markdown
Contributor

@tungv tungv commented Sep 10, 2017

calling chokidar's watch() on a same file multiple times doesn't
necessarily create multiple background watchers. These calls and
watcher.close() calls might cause a race-condition bug that stops the
seemingly newly created watcher.

this fix will not remove the need to use debounce because we need to
make sure multiple file changes happen simultaneously (usually due to a
build pipeline) will not result in an inconsistent state.

calling chokidar's `watch()` on a same file multiple times doesn't
necessarily create multiple background watchers. These calls and
`watcher.close()` calls might cause a race-condition bug that stops the
seemingly newly created watcher.

this fix will not remove the need to use debounce because we need to
make sure multiple file changes happen simultaneously (usually due to a
build pipeline) will not result in an inconsistent state.
@tungv tungv mentioned this pull request Sep 10, 2017
Copy link
Copy Markdown
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Seems alright 👍

@zigomir
Copy link
Copy Markdown
Contributor

zigomir commented Sep 11, 2017

Awesome @tungv! I can confirm this really fixes #23 and probably #28 which seems like a duplicated issue.

@tungv
Copy link
Copy Markdown
Contributor Author

tungv commented Sep 12, 2017

@leo can we merge this?

@timneutkens timneutkens merged commit 3d264d5 into vercel:master Sep 12, 2017
@timneutkens
Copy link
Copy Markdown
Member

Done @tungv, @leo needs to release it 👍

@tungv
Copy link
Copy Markdown
Contributor Author

tungv commented Sep 12, 2017

great! Can we also merge #35 before releasing?

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