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

tinyglobby can hang on invalid fast-glob backslash usage #72

Closed
userquin opened this issue Nov 12, 2024 · 21 comments
Closed

tinyglobby can hang on invalid fast-glob backslash usage #72

userquin opened this issue Nov 12, 2024 · 21 comments

Comments

@userquin
Copy link

Check vuejs/vitepress#4357

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Nov 12, 2024

i'm on windows and absolute patterns should work, can you provide a minimal repro? whatever it is if it's perf related #69 will probably solve it

@userquin
Copy link
Author

userquin commented Nov 12, 2024

clone VP (VitePress) repo then pnpm install && pnpm build

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Nov 12, 2024

it's using an absolute path with \s, which normally shouldn't be used in glob patterns in neither fast-glob or tinyglobby (https://github.com/mrmlnc/fast-glob#pattern-syntax). will investigate how fast-glob handles them when the pattern is absolute though

image

@userquin
Copy link
Author

userquin commented Nov 12, 2024

FYI: I added fast-glob in VP repo pnpm add -D fast-glob -w changing the rollup.config.ts import and usage and it works:

// foo.js => node ./foo.js
import { resolve, normalize } from 'node:path'
import { fileURLToPath } from 'node:url'
import { globSync } from 'tinyglobby'

const ROOT = fileURLToPath(import.meta.url)
const r = (p) => normalize(resolve(ROOT, '..', p))

console.log(r('src/node/worker_*.ts'))
console.log(
  globSync('src/node/worker_*.ts', {
    cwd: r('.'),
    onlyFiles: true,
    expandDirectories: false
  })
)
console.log('RUNNING')

@SuperchupuDev SuperchupuDev changed the title tinyglobby hangs when using absolute paths in patterns on Windows tinyglobby inconsistent to fast-glob with absolute patterns that have \ Nov 12, 2024
@userquin
Copy link
Author

https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#how-to-write-patterns-on-windows

fast-glob handles that, but doesn't recommend

Using simple replacement works:

imagen

@SuperchupuDev
Copy link
Owner

will try to make it as consistent to fast-glob as possible

@Torathion
Copy link
Contributor

Any news on this? This is probably the biggest reason that prevents many tools from switch from fast-glob to tinyglobby.

@SuperchupuDev
Copy link
Owner

is it? thought it wasn't a priority since the issue author's linked repo no longer has the problem. can take a deeper look into it for the next release that i want to release ASAP

@Torathion
Copy link
Contributor

From what I've gathered: The windows glob issues are very important. OP came from vitepress having this issue and remix and analog also referring to this issue. I wanted to make a PR in npm-check-updates and came across the same issue as a dozen tests started to fail after switching from fast-glob to tinyglobby.

@Sytten
Copy link

Sytten commented Feb 13, 2025

I would also say yes this is an issue, we use tsup to build plugins in our ecosystem and I got a report from a user that the build process was hanging on windows. It does look like it is related to tinyglobby.

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Feb 13, 2025

sorry to keep y'all waiting, the next release is currently being blocked by some perf implementation (currently broken on main) that i've been trying to figure out for a while. once that's solved i'll fix this and release a new version

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Feb 14, 2025

the perf implementation has been merged! so i'm working on this right now, but i can't seem to find a single pattern with \ as separators that make fast-glob work, on either windows or linux. @Torathion @userquin can y'all share a minimal reproduction of a case in which fast-glob returns results correctly like that? to then change tinyglobby so that it matches fast-glob's functionality

@Torathion
Copy link
Contributor

Unfortunately I can't help you with this as I'm not very familiar with globs. I only found out about it when the tests in NCU suddenly failed after using it as a drop-in replacement.

But I see that you only have tests using /, while you don't have any tests using \\ alone. Maybe adding these tests would help to establish tinyglobby as drop-in replacement for fast-glob.

@userquin
Copy link
Author

userquin commented Feb 15, 2025

@SuperchupuDev here small reproduction using both: https://github.com/userquin/tinyglobby-issue-72

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Feb 15, 2025

@userquin fg.mjs doesn't seem to return anything in your repro. is there any case in which fast-glob returns something with windows paths as likely expected, or is the issue that tinyglobby doesn't return []? i thought the expected thing was support for those patterns (i don't have windows, screenshot is from a friend, sorry for the quality)

Image

@userquin
Copy link
Author

Yeah, fg.mjs doesn't hang while tg.mjs does (the original issue)

https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#how-to-write-patterns-on-windows

@SuperchupuDev SuperchupuDev changed the title tinyglobby inconsistent to fast-glob with absolute patterns that have \ tinyglobby can hang on invalid fast-glob backslash usage Feb 16, 2025
@SuperchupuDev
Copy link
Owner

okay, so as far as i understand the problem you all are having is that that invalid fast-glob usage hangs, but it's invalid fg usage regardless. is that right?

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Feb 16, 2025

in theory the new optimizer i merged a few days ago should make this not hang anymore (in fact we tried and it doesn't hang while it did on 0.2.10), but it still creates an odd internal state regardless

@SuperchupuDev
Copy link
Owner

@userquin tinyglobby 0.2.11 is out which should fix the perf problems thanks to new optimizations introduced. can you tell me if it still hangs? locally reproducing it doesn't seem to hang anymore, at least on a friend's computer

@userquin
Copy link
Author

@SuperchupuDev now it works:

Image

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

No branches or pull requests

4 participants