fix(deps): replace glob usage with fast-glob #6384
Conversation
mrstork
force-pushed
the
fix/upgrade-glob
branch
3 times, most recently
from
8983e60 to
be136d8
Compare
mrstork
changed the title
| cwd: baseDir, | ||
| }) | ||
|
|
||
| result.push(...resolved) | ||
| // esbuild expects relative paths, but tinyglobby uses `posix.normalize()` which strips leading `./` |
There was a problem hiding this comment.
Credit to @serhalp for figuring this one out
There was a problem hiding this comment.
This comment is about tinyglobby (from my other PR), but here we're switching to fast-glob.
AFAICT from reading the fast-glob source and tests, this shouldn't be needed, I think. I wonder if this is resulting in ././foo which happens to still work fine?
There was a problem hiding this comment.
Yeah let's figure out if that's the case and avoid the duplicated segments if possible.
There was a problem hiding this comment.
Without this code the test included_files with multiple glob stars are correctly resolved before passing to esbuild fails because lang/en.json is missing the leading ./. I'll update the comment though.
| "filter-obj": "^6.0.0", | ||
| "find-up": "^7.0.0", | ||
| "glob": "^8.0.3", |
There was a problem hiding this comment.
For anyone else reviewing this:
| @@ -17,8 +17,7 @@ export const getSideFiles = async function (functionPath: string, stat: Stats): | |||
| const paths = await glob(`${functionPath}/**`, { | |||
| absolute: true, | |||
| cwd: functionPath, | |||
| ignore: `**/node_modules/**`, | |||
| nodir: true, | |||
There was a problem hiding this comment.
👍🏼 fast-glob onlyFiles is true by default, I see: https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#onlyfiles
| cwd: baseDir, | ||
| }) | ||
|
|
||
| result.push(...resolved) | ||
| // esbuild expects relative paths, but tinyglobby uses `posix.normalize()` which strips leading `./` |
There was a problem hiding this comment.
This comment is about tinyglobby (from my other PR), but here we're switching to fast-glob.
AFAICT from reading the fast-glob source and tests, this shouldn't be needed, I think. I wonder if this is resulting in ././foo which happens to still work fine?
Summary
This work started off with remaking #6143 (i.e. upgrading glob to the highest version we can support) but turned into simply trying to remove the package from our dependencies instead with the aim of reducing our package size.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)