privatenumber / tsx

⚡️ TypeScript Execute | The easiest way to run TypeScript in Node.js
https://tsx.is
MIT License
9.8k stars 154 forks source link

fix: Allow ignore patterns to apply to absolute path instead of relative path #656

Open kingston opened 2 months ago

kingston commented 2 months ago

Addresses #221

kingston commented 2 months ago

@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: 1) Executed file + dependencies watcher (basically what we have today with this PR) 2) If --include is present, includes-only watcher. Watch all glob-parents and apply --include globs to filter out only the files we care about. (would coincidentally also address https://github.com/privatenumber/tsx/pull/643)

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.

privatenumber commented 2 months ago

That sounds good to me! Thanks @kingston

Will try to update the docs soon.

kingston commented 2 months ago

@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.

kingston commented 2 months ago

@privatenumber I don't have access to a windows instance but just uploaded a fix that should address the failing test.

kingston commented 2 months ago

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.

privatenumber commented 2 months ago

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.

kingston commented 1 month ago

@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.

C:\...\tsx\docs\getting-started.md
  62:14  error  Parsing error: Unexpected token :

C:\...\tsx\docs\typescript.md
  108:14  error  Parsing error: Unexpected token :
  123:14  error  Parsing error: Unexpected token :

My guess is that the glob pattern for ignoring files also doesn't work on Windows :P

lintroll --node --cache --ignore-pattern 'docs/*.md' .

kingston commented 1 month ago

@privatenumber just wanted to follow up - are there any other action items for this PR?

privatenumber commented 1 month ago

Sorry @kingston been swamped lately. I'll try to take a look Sunday or next week.

kingston commented 1 month ago

Sure no problem - I get it.

privatenumber commented 1 month ago

I pulled out your docs changes into master so we can get it out faster. Thanks!