Skip to content

Conversation

@thecodrr
Copy link
Owner

Fixes #143

This PR fixes multiple issues with the queue/enqueue logic ensuring:

  • onQueueEmpty is called only once
  • The crawler aborts on error (before it kept crawling which was pointless)

Another change I had to make was how the crawler always returned results maxDepth + 1 deep. This might break some things (but not really sure since all tests pass).

cc @SuperchupuDev can you test this out with tinyglobby and let me know if something breaks?

)
return;

this.pushDirectory(directoryPath, paths, filters);
Copy link
Contributor

@43081j 43081j May 23, 2025

Choose a reason for hiding this comment

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

does this change mean the root directory isn't pushed anymore? presumably directoryPath can be /, so this change would mean we push the child directories but not / itself.

that may be right, just checking i understand the logic and if this is a change in behaviour

@43081j
Copy link
Contributor

43081j commented May 23, 2025

looks good to me!

it may be worth adding a test for crawl("/") and maxDepth: 0 since thats what uncovered some of this in the first place

pretty sure we already test maxDepth elsewhere, just not at the root

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented May 25, 2025

hi @thecodrr! thanks for fixing this so fast. i haven't been able to test this due to not having a windows machine nearby and the lack of preview builds. it'd be easy to do that if #124 happened (in which case let me know, i can easily open a pr to do it)

@SuperchupuDev
Copy link
Contributor

@thecodrr tinyglobby@main tests seem to not be negatively affected by this fix, thank you @43081j for testing

@thecodrr thecodrr merged commit 2f6145e into master May 28, 2025
11 checks passed
@thecodrr thecodrr deleted the fix/multiple-resolve-calls branch May 28, 2025 04:57
@SuperchupuDev
Copy link
Contributor

hey @thecodrr, it turns out this does break tests related to maxDepth, sorry for not finding out earlier 😅. thankfully, it's really easy to fix! i've opened a pr at #149

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.

withPromise promise is resolved multiple times which causes hangs on windows

4 participants