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

Replace glob by tinyglobby #647

Merged
merged 7 commits into from
Feb 15, 2025
Merged

Replace glob by tinyglobby #647

merged 7 commits into from
Feb 15, 2025

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented Feb 15, 2025

This should I think fix the support for Node.js v12 (and fixes #646)

From what I can see, Node.js v12 broken (at least) because of minimatch v9 being used, which only supports 14 and up. minimatch is used because glob@10, so switching to tinyglobby (v12 and up) might be a good solution to the problem actually.
(#646 (comment))

Note: I haven't tested it as thoroughly as I'd like to yet, but it seems to work well so far.

The packages are relatively interchangeable, though the options change; Here is the documentation for each:

nodir becomes onlyFiles: true
nocase becomes caseSensitiveMatch (but inverted)
I could not find an equivalent to glob's mark (Append a / on any directories matched) but that is the default behavior for tinyglobby anyway.

From what I can tell, these are the paths through which globbing is used (My understanding of what is happening at least):
In resolveGlobs, we look for test files. By default we look for <root>/tests, but that can be overridden by CLI arguments (elm-test otherDir/ tests/SpecificTest.elm).We first iterate through that list to understand which each of these paths exist or not. If it exists, we defer to findAllElmFilesInDir if it's a directory, and just take the file it it's a file. If it does not exist, then it's likely a glob (elm-test "**/*Test.elm" "te*ts/", which we defer to resolveCliArgGlob where we apply the glob. For every directory, we then again defer to findAllElmFilesInDir`.

In findAllElmFilesInDir`, we use globbing to find all contained Elm files.

@jfmengels jfmengels force-pushed the tinyglobby branch 2 times, most recently from b0eb52a to fd7526c Compare February 15, 2025 14:27
absolute: true,
ignore: ignoredDirsGlobs,
nodir: true,
onlyFiles: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onlyFiles: true is the default, but I've kept it for explicitness.

Copy link
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Thank you so much! ⭐

I think I have to do some manual tests on Windows before I dare merging this.

.flatMap((filePath) =>
filePath.endsWith('/') ? findAllElmFilesInDir(filePath) : filePath
);
return globSync(pattern, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: there a big comment above about glob@8 and Windows and backslashes. Try it out on Windows to see how it behaves in tinyglobby, and remove or update the code/comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s needed: 7814ff7 (#647)

Updated the comment: 46ca4aa (#647)

onlyFiles: false,
}).flatMap((filePath) =>
// Directories have their path end with `/`
filePath.endsWith('/') ? findAllElmFilesInDir(filePath) : filePath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: Does it end with a slash on Windows? We don’t seem to have test coverage for this condition being true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added test case: 36cdb47 (#647)

Btw, that test was failing on master – see #648. But it passed when installing glob 8 instead of 10. Another breakage by glob 🙈

@lydell lydell mentioned this pull request Feb 15, 2025
@lydell lydell merged commit 352dce5 into rtfeldman:master Feb 15, 2025
20 checks passed
@jfmengels jfmengels deleted the tinyglobby branch February 15, 2025 23:36
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.

Consider switching to tinyglobby
2 participants