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

Regression: v2.1.0 makes folder globbing impossible in vitest.workspace.ts (No loader is configured for ".gitignore" files: .gitignore) #6553

Open
6 tasks done
kahagerman opened this issue Sep 23, 2024 · 24 comments
Labels
documentation Improvements or additions to documentation p3-minor-bug An edge case that only affects very specific usage (priority) upstream

Comments

@kahagerman
Copy link

kahagerman commented Sep 23, 2024

Describe the bug

I am aware that v2.1.0 made backwards incompatible changes, and that this was intentional.

It appears to be impossible (since v2.1.0) to glob for folders instead of files in vitest.workspace.ts.

The documentation states that this is valid configuration:

export default [
  'packages/*'
]

This appears to be incorrect, as it will now match any files in packages/ as a config file (i.e. packages/.gitignore, etc.)

I believe that this should be updated to read:

export default [
  'packages/*/'
]

Unfortunately this also does not work; it is additionally matching other files within the sub-folder as configuration files. (i.e.packages/package-1/.gitignore)

Reproduction

Previously (before v2.1.0), my vitest.workspace.ts contained:

export default ["./*"];

So all direct sub-folders (one level down) were considered packages in the workspace.

Today (when upgrading to ^2.1.0)

This works:

export default ["*/*vitest*"];

But the following all break (with some variant of No loader is configured for ".gitignore" files: .gitignore):

  • original, broken as expected by v2.1.0
    export default ["./*"];
  • Same but ending with /. I think we should expect this to only match folders, not files in those folders?
    export default ["./*/"];
  • Excluding ./ but ending with /. I think we should expect this to only match folders, not files in those folders?
    export default ["*/"];

System Info

`vitest ^2.1.0`

Tested with vitest `2.1.1`.

Used Package Manager

pnpm

Validations

@sheremet-va
Copy link
Member

The glob you mentioned is correct since it only matches the first level assuming you only have folders there:

Vitest will consider every folder in packages as a separate project even if it doesn't have a config file inside

Copy link

Hello @kahagerman. Please provide a minimal reproduction using a GitHub repository or StackBlitz (you can also use examples). Issues marked with needs reproduction will be closed if they have no activity within 3 days.

@kahagerman
Copy link
Author

The glob you mentioned is correct since it only matches the first level assuming you only have folders there:

Vitest will consider every folder in packages as a separate project even if it doesn't have a config file inside

This is not actually the issue I am reporting, though it is related.

Typically when globbing, a trailing / indicates to only match folders, not files.

This is not being respected (seems like every file under this path is matched), so there is no way to match only folders, and thus no way to restore the old behaviour.

@treardon17
Copy link

I'm having a similar problem but the issue is with .DS_Store files:

No loader is configured for ".DS_Store" files: .DS_Store

My vitest workspace is configured as:

import { defineWorkspace } from 'vitest/config';

export default defineWorkspace([
  'packages/*',
]);

This configuration works in 2.0.5, but breaks in 2.1.0 and 2.1.1. I'm running macOS 15. This is notably absent when running the same command in a github workflow, presumably because .DS_Store files are macOS specific.

@kahagerman
Copy link
Author

@treardon17 Note that there was a breaking change made for workspaces in v2.1.0, check the release notes.

@sheremet-va
Copy link
Member

sheremet-va commented Sep 25, 2024

This is not actually the issue I am reporting, though it is related.

Typically when globbing, a trailing / indicates to only match folders, not files.

This is not being respected (seems like every file under this path is matched), so there is no way to match only folders, and thus no way to restore the old behaviour.

This is just not true. The glob that matches only folders works correctly with / and without / at the end: https://stackblitz.com/edit/vitest-dev-vitest-9cztj4?file=vitest.workspace.ts&initialPath=__vitest__/

The glob ./* will match all the files and folders in the working directory.

Typically when globbing, a trailing / indicates to only match folders, not files.

The tinyglobby package also matches files; this might be an upstream issue.

@kahagerman
Copy link
Author

kahagerman commented Sep 25, 2024

This is not actually the issue I am reporting, though it is related.
Typically when globbing, a trailing / indicates to only match folders, not files.
This is not being respected (seems like every file under this path is matched), so there is no way to match only folders, and thus no way to restore the old behaviour.

This is just not true. The glob that matches only folders works correctly with / and without / at the end: stackblitz.com/edit/vitest-dev-vitest-9cztj4?file=vitest.workspace.ts&initialPath=vitest/

This fails if a plain file exists in packages/.

Here the only thing I changed was to add an empty packages/.gitignore file (updated stackblitz):
Screenshot 2024-09-25 at 9 14 50 AM

@sheremet-va
Copy link
Member

And it fails correctly because the file matches your glob. If Vitest sees a file, it assumes that it’s a Vitest config. This is mentioned in the documentation.

@kahagerman
Copy link
Author

kahagerman commented Sep 25, 2024

No, packages/*/ should not match packages/.gitignore

@sheremet-va
Copy link
Member

sheremet-va commented Sep 25, 2024

No, packages/*/ should not match packages/.gitignore

As I said before:

The tinyglobby package also matches files <when glob ends with "/">; this might be an upstream issue.

But there is no issue with a glob mentioned in the documentation when it is used correctly.

@kahagerman
Copy link
Author

kahagerman commented Sep 25, 2024

Ok, I see what you're saying now; I misunderstood your original comment.

I wonder if it's worth using a more mature glob library if tinyglobby doesn't support this today.

@sheremet-va
Copy link
Member

My thoughts on this:

  • .DS_Store is a bug, we should ignore it by default
  • Not respecting the / at the end is a bug
  • Glob logic itself works correctly, but it will be better to change the packages/* to packages/*/ in the example

@sheremet-va sheremet-va added documentation Improvements or additions to documentation upstream p3-minor-bug An edge case that only affects very specific usage (priority) and removed needs reproduction labels Sep 25, 2024
@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 25, 2024

investigating the trailing / issue it looks like fast-glob has really inconsistent behavior around trailing / in patterns and the behavior of only returning directories is only present if the pattern is dynamic (and otherwise fast-glob matches files too). because of this, i'm not sure if fixing this (and therefore making tinyglobby inconsistent as well) would be a good idea

you can test this inconsistency in fast-glob this way:

import glob from 'fast-glob';

// dynamic pattern, only returns folders so no files are matched
await glob(['*.ts/'], {
  onlyFiles: false
});

// static pattern, returns files too
await glob(['index.ts/'], {
  onlyFiles: false
});

@sheremet-va
Copy link
Member

you can test this inconsistency in fast-glob this way

I think this is just a bug in fast-glob. Personally, I don't expect any files to be matched with a glob ending with /

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 25, 2024

yeah but i imagine making tinyglobby not returning files if the pattern ends with / in a consistent way would probably break way more users and existing usecases

@kahagerman
Copy link
Author

kahagerman commented Sep 25, 2024

Would it make sense to upstream this issue to fast-glob, and see whether they consider it to be a bug there?

@sheremet-va
Copy link
Member

sheremet-va commented Sep 25, 2024

yeah but i imagine making tinyglobby not return files if the pattern ends with / in a consistent way would probably break way more users and existing usecases

Can you give an example of a use case that it breaks? I didn't find any tools online that support having a / at the end for file paths

Here is an example of this exact issue in node-glob where it was considered a bug: isaacs/node-glob#158

Here is a micromatch result:

> require('micromatch').isMatch('/hello/world.js', '**/*.js')
true
> require('micromatch').isMatch('/hello/world.js', '**/*.js/')
false
> require('micromatch').isMatch('/hello/world.js', '/hello/world.js/')
false
> require('micromatch').isMatch('/hello/world.js', '/hello/world.js')
true

@SuperchupuDev
Copy link
Contributor

basically what i mentioned above, using a pattern like src/index.ts/ which currently matches src/index.ts (assuming it's a file) in both fast-glob and tinyglobby. i'm not sure how often that's used but i imagine it's used more frequently than a dynamic pattern leading with a /

@sheremet-va
Copy link
Member

sheremet-va commented Sep 25, 2024

i imagine it's used more frequently than a dynamic pattern leading with a /

Can you explain why you think that is the case? I don't see why people would use it that way since other tools disagree (micromatch)

@kahagerman
Copy link
Author

kahagerman commented Sep 25, 2024

Arguably, **/*.js/ should never match /hello/world.js, since there is no sequence of characters that you can replace the *'s with to make these identical.

**/ would be the exception that proves the rule, in that ** specifically means "match the following at any number of directories down, including the current one", so ./**/foo must match ./foo.
I suspect what we're seeing here is a bug related to this^ exception.

@sheremet-va
Copy link
Member

The new built-in fs.glob API also returns only folders correctly (tested with a Vitest repo):

> require('fs').globSync('./shims.d.ts')
[ 'shims.d.ts' ]
> require('fs').globSync('./shims.d.ts/')
[]
> require('fs').globSync('./*/')
[
  'test',
  'scripts',
  'patches',
  'packages',
  'node_modules',
  'examples',
  'docs'
]

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 25, 2024

how does fs.glob handle patterns without trailing slashes that refer to directories (i.e. a "src" pattern)? all i know is that fast-glob matches directories without needing a trailing slash on the pattern

@sheremet-va
Copy link
Member

sheremet-va commented Sep 25, 2024

how does fs.glob handle patterns without trailing slashes that refer to directories (i.e. a "src" pattern)? all i know is that fast-glob matches directories without needing a trailing slash on the pattern

Like you would expect:

> require('fs').globSync('./patches')
[ 'patches' ]
> require('fs').globSync('./patches/')
[ 'patches' ]
> require('fs').globSync('patches')
[ 'patches' ]

all i know is that fast-glob matches directories without needing a trailing slash on the pattern

And it is the correct behaviour. The bug is when it doesn't treat the src/ the same as src

@SuperchupuDev
Copy link
Contributor

just opened mrmlnc/fast-glob#458

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation p3-minor-bug An edge case that only affects very specific usage (priority) upstream
Projects
None yet
Development

No branches or pull requests

4 participants