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

Upgrade glob to v10 #404

Closed
wants to merge 9 commits into from
Closed

Upgrade glob to v10 #404

wants to merge 9 commits into from

Conversation

gurgunday
Copy link
Member

Checklist

@gurgunday
Copy link
Member Author

Only 1 test is failing

@gurgunday
Copy link
Member Author

gurgunday commented Sep 4, 2023

Windows is failing for now

Edit: not sure how to fix it

@@ -345,7 +343,7 @@ async function fastifyStatic (fastify, opts) {
const winSeparatorRegex = new RegExp(`\\${path.win32.sep}`, 'g')

for (const rootPath of Array.isArray(sendOptions.root) ? sendOptions.root : [sendOptions.root]) {
const files = await globPromise(path.join(rootPath, globPattern).replace(winSeparatorRegex, path.posix.sep), { nodir: true, dot: opts.serveDotFiles })
const files = await glob(path.join(rootPath, globPattern).replace(winSeparatorRegex, path.posix.sep), { follow: true, nodir: true, dot: opts.serveDotFiles })
Copy link
Contributor

Choose a reason for hiding this comment

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

check what happens here, when glob is fed with the windows related options

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 4, 2023

I ping @isaacs, maybe he has some hint on how to fix it. Maybe using posix: true?

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 4, 2023

Time to install windows.

@Uzlopak Uzlopak added the semver-major Issue or PR that should land as semver major label Sep 4, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 4, 2023

@gurgunday
Can you come online on discord, please.

@Fdawgs
Copy link
Member

Fdawgs commented Sep 5, 2023

This isn't possible as glob dropped support for node 14 in v9.x and we're still supporting (and testing against) node 14 in the CI. :(

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 5, 2023

node 14 tests pass so far and this has to be a semver major anyway.

@gurgunday
Copy link
Member Author

gurgunday commented Sep 5, 2023

Yeah even after purging Windows from all my machines, it seems to still cause me trouble 🤠

Without a hint on why things aren't working, I can only keep commit-guessing 🤣

@gurgunday gurgunday marked this pull request as draft September 6, 2023 08:26
@gurgunday
Copy link
Member Author

I'll try again on a Windows machine

@gurgunday gurgunday closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants