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

Watch target files with Chokidar #30

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

Conversation

blake-mealey
Copy link

@blake-mealey blake-mealey commented Nov 5, 2019

Fixes: #5

New PR for watching targeted files, this time using Chokidar as we discussed.

We discussed using process.on('exit', ...) to close watchers, but I found this wasn't feasible because that requires sync operations and closing Chokidar is async. I made an issue on Chokidar and they said there's no need to close them manually (see paulmillr/chokidar#945).

@@ -251,16 +348,16 @@ describe('Options', () => {
expect(console.log).toHaveBeenCalledTimes(5)
expect(console.log).toHaveBeenCalledWith(green('copied:'))
expect(console.log).toHaveBeenCalledWith(
green(` ${bold('src/assets/asset-1.js')} → ${bold('dist/asset-1.js')}`)
green(` ${bold('src/assets/asset-1.js')} → ${bold(join('dist', 'asset-1.js'))}`)
Copy link
Author

Choose a reason for hiding this comment

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

This test was broken on Windows because it was getting 'dist\asset-1.js' instead of 'dist/asset-1.js'

join fixes this by using the separator from the OS

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, so it outputs backslashes on windows 🤔

I'll try to resolve this in src code so paths won't be different in mac/windows since this doesn't match with target.src ones.

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #30 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #30   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          36     49   +13     
  Branches       12     14    +2     
=====================================
+ Hits           36     49   +13
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57df513...4d6431c. Read the comment docs.

@vladshcherbin
Copy link
Owner

Time to check this one, thank you for helping :)

await copyFiles(copyTargets, verbose, copyOptions)
}

return chokidar.watch(src, { ignoreInitial: true })
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, we can't pass src to chokidar. Chokidar v3 uses anymatch, which uses picomatch inside:

chokidar -> anymatch -> picomatch

This package uses globby inside for glob support. Globby uses fast-glob inside, which uses micromatch:

globby -> fast-glob -> micromatch -> picomatch

So, chokidar v3 misses micromatch package which adds support for some glob features on top of picomatch. Globs will have different features for copy and watch. Chokidar used micromatch in v2, but switched in v3.

Copy link
Author

Choose a reason for hiding this comment

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

Well that's unfortunate... I'll think about it a bit and do some research into options.

Copy link
Owner

Choose a reason for hiding this comment

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

I've opened an issue in chokidar paulmillr/chokidar#956, maybe only braces extension is missing and we can live with that.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems to be compatible. I'll add few tests this week and hopefully release a new version after merging this.

@vladshcherbin
Copy link
Owner

I've checked and tested this a bit. Everything is fine, except glob support. Chokidar uses picomatch inside which has less features than glob package used for copying here.

I'm not sure how to solve this, need to find a way chokidar use micromatch like it was in v2. Maybe it's possible to pass glob library instance or smth like this.

Details

@bennypowers
Copy link

This can be accomplished with rollup's addWatchFile API, no?

@blake-mealey
Copy link
Author

@bennypowers unfortunately that didn't fit all requirements, see: #27 (comment)

@vladshcherbin any update on merging this?

@blake-mealey
Copy link
Author

Bump 😃

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.

Support rollup's watch mode
3 participants