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

Fixed unreliable hot reloading #29

Merged
merged 2 commits into from
Aug 26, 2017
Merged

Fixed unreliable hot reloading #29

merged 2 commits into from
Aug 26, 2017

Conversation

tungv
Copy link
Contributor

@tungv tungv commented Aug 25, 2017

  1. remove cwd option to yield absolute paths
  2. add debounce to avoid multiple restarts happen unexpectedly when more than one file change

1. remove cwd option to yield absolute paths
2. add debounce to avoid multiple restarts happen unexpectedly when more than one file change
@tungv tungv mentioned this pull request Aug 25, 2017
@leo leo changed the title fix: unreliable hot reload Fixed unreliable hot reloading Aug 26, 2017
@leo
Copy link
Contributor

leo commented Aug 26, 2017

Looks really good! But how about using a package for the debouncing? The function looks like it could be abstracted away with a package.

@tungv
Copy link
Contributor Author

tungv commented Aug 26, 2017

I initially used lodash, but worried about the size of the package. What do you think about lodash/debounce?

@leo
Copy link
Contributor

leo commented Aug 26, 2017

@tungv I'd rather go with a package that's only doing debouncing, nothing else.

@tungv
Copy link
Contributor Author

tungv commented Aug 26, 2017

Ok Leo, I will update in a few hours. Thanks!

@tungv
Copy link
Contributor Author

tungv commented Aug 26, 2017

hi @leo, I opted to use debounce npm package.

their code looks very clean https://unpkg.com/debounce@1.0.2

@leo leo merged commit 49e1c8d into vercel:master Aug 26, 2017
@leo
Copy link
Contributor

leo commented Aug 26, 2017

Thanks a lot!

@tungv
Copy link
Contributor Author

tungv commented Aug 27, 2017

Great. Thank you for merging this over the weekend. I'm eager to use the new version next week.

@bakpakin
Copy link
Contributor

bakpakin commented Sep 2, 2017

It seems like this merge makes the --ignore option no longer work properly, reverting #24

@tungv
Copy link
Contributor Author

tungv commented Sep 2, 2017

it's true. --ignore with relative paths is broken. How about resolving ignored paths before passing them to chokidar?

@bakpakin
Copy link
Contributor

bakpakin commented Sep 2, 2017

That requires detecting if a pattern is resolvable. Patterns like **/tmp/** are valid, but should not be resolved with path.resolve. Essentially, chokidar ignores anymatch patterns, not directories.

@tungv
Copy link
Contributor Author

tungv commented Sep 4, 2017

maybe we should explicitly say that ignore pattern are not relative to cwd.

@zigomir
Copy link
Contributor

zigomir commented Sep 8, 2017

Interestingly, this doesn't fix hot reloading issue for me. Still same thing as described in #23 :/

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

4 participants