fix: Allow ignore patterns to apply to absolute path instead of relative path #656
Conversation
…ive path + Upgrade chokidar to v4 for a smaller footprint + Add picomatch for glob matching + Add test for parent directory ignores
|
@privatenumber just kicking this off as an initial draft PR so we can comment on raw code vs. ideas. Right now all tests pass except one (--include with glob patterns). This is because chokidar v4 removes all glob support including include patterns. This is a bit trickier to resolve since we'd need to find all the files that match that glob which gets a lot messier/more complex. We can either stay with chokidar v3 or do something a bit more complex. Looking at chokidar v3, it seems like when they encounter a glob pattern they just take the glob-parent and watch the highest parent of the glob e.g. for /blah/foo/**/test - they watch /blah/foo and filter by **/test. So we can try to mimic this behavior manually. We could potentially do two separate watchers:
I don't think it adds too much of an overhead and simplifies the logic. On a side note, I realized the docs were out of date with --exclude / --include (https://github.com/privatenumber/tsx/blob/master/docs/watch-mode.md) but can be a separate issue to resolve. |
|
That sounds good to me! Thanks @kingston Will try to update the docs soon. |
|
@privatenumber I've updated the PR to now pass all tests including the include tests. I introduced 2 additional libraries to get the glob parent and check if it is a glob or not (otherwise glob parent would remove the ending). Overall, I think it should be good to go. However, there's one scenario where I'm unclear what the behavior should be. If you pass a directory to --exclude without any glob patterns, should it automatically match all its children? e.g. --exclude ./foo will match ./foo/? If so, we'd probably need to re-work the logic to create a separate matcher for the exact path and path with / appended to it for non-glob patterns. Also left a small update the docs to make it more accurate. |
|
@privatenumber I don't have access to a windows instance but just uploaded a fix that should address the failing test. |
|
Hmm.. that didn't seem to work. I'll try to see if I can get a windows machine to test it on since I have a Mac. |
|
I actually don't have a Windows machine either. It's not the most ergonomic environment but I just make a separate branch, update the GitHub Actions CI to only run on Windows, and debug there 😅 I'll have more time this weekend so I can take a closer look/help out. |
|
@privatenumber OK, I spun up a Windows VM on my Mac and was able to debug more ergonomically :). It looks like globs and Windows paths are complicated since globs are technically not allowed to have \ in them so I used a library normalize-paths (used by chokidar v3 as well) to normalize the paths to / since chokidar v4 also does that when passing to ignored. This makes all tests pass on my Windows 11 VM :). Fun side-note: I was unable to commit to the repo because of the lint pre-commit hook since the lint command doesn't work on Windows so I had to skip the hooks when committing. My guess is that the glob pattern for ignoring files also doesn't work on Windows :P
|
|
@privatenumber just wanted to follow up - are there any other action items for this PR? |
|
Sorry @kingston been swamped lately. I'll try to take a look Sunday or next week. |
|
Sure no problem - I get it. |
| || isOptionsExclude(file) | ||
| ) | ||
| ), | ||
| }).on('all', reRun); |
There was a problem hiding this comment.
Is it possible to add to the same watcher above instead of making a new one?
| return normalizePath(pattern); | ||
| } | ||
| return normalizePath(path.join(process.cwd(), pattern)); | ||
| }; |
There was a problem hiding this comment.
This only seems used to normalize the path slashes in user provided paths—do they need need to be normalized?
| }, | ||
| ).on('all', reRun); | ||
|
|
||
| if (resolvedIncludes.length > 0) { | ||
| const globParents = resolvedIncludes.map(pattern => ( | ||
| isGlob(pattern) |
There was a problem hiding this comment.
This seems like a pretty hefty function to run only to check if it's a glob. What if we just run globParent on all inputs regardless of whether they're a glob?
| @@ -4,7 +4,10 @@ import { constants as osConstants } from 'node:os'; | |||
| import path from 'node:path'; | |||
| import { command } from 'cleye'; | |||
| import { watch } from 'chokidar'; | |||
| import picomatch from 'picomatch'; | |||
There was a problem hiding this comment.
Picomatch is actually the largest dependency now (bundle size):
https://pkg-size.dev/chokidar%20glob-parent%20is-glob%20normalize-path%20picomatch
Is there a lighter alternative?
…gnore-absolute-patterns
|
I pulled out your docs changes into |
Addresses #221