build icon indicating copy to clipboard operation
build copied to clipboard

Published 20 hours ago • verified publisher icon netlify

chore(deps): switch from fast-glob to tinyglobby

Open benmccann opened this issue 6 months ago • 2 comments

🎉 Thanks for submitting a pull request! 🎉

Summary

https://npmgraph.js.org/?q=fast-glob - 17 dependencies https://npmgraph.js.org/?q=tinyglobby - 2 dependencies

I see https://github.com/netlify/build/pull/6094 tried to switch both fast-glob and glob. However, tinyglobby is currently only a replacement for globby and fast-glob. I made a request for glob support awhile back here: https://github.com/SuperchupuDev/tinyglobby/issues/97. It might not be implemented anytime soon if at all, so it'd be nice to migrate just fast-glob in the meantime, which is still a nice win in terms of dependency size.

For us to review and ship your PR efficiently, please perform the following steps:

  • [ ] Open a bug/issue before writing your code 🧑‍💻. This ensures 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.
  • [x] Read the contribution guidelines 📖. This ensures your code follows our style guide and passes our tests.
  • [x] Update or add tests (if any source code was changed or added) 🧪
  • [ ] Update or add documentation (if features were changed or added) 📝
  • [ ] Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

benmccann avatar May 21 '25 16:05 benmccann

Hello @benmccann, I've just merged #6384 which replacing use of glob with fast-glob, so there are a few more occurrences to replace with tinyglobby. Now that we're only making use of one glob library, replacement should be even more impactful 👍

mrstork avatar Jun 04 '25 23:06 mrstork

Unfortunately https://github.com/netlify/build/pull/6384 made this much harder because it introduced the use of the globstar option, which is not present in tinyglobby. While I haven't investigated in detail, I suspect that's why the tests are failing. I'll file a request in that repo and see if the author has any ideas in terms of work around or whether it's hard to implement and will reference this issue, so you can find it

Failing tests ``` ❯ tests/symlinked_included_files.test.ts:59:47 57| 58| // expect that the symlink for `node_modules/crazy-dep` is preserved 59| expect(await readDirWithType(unzippedPath)).toEqual({ | ^ 60| '___netlify-bootstrap.mjs': false, 61| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/3]⎯

FAIL tests/symlinked_included_files.test.ts > preserves multiple symlinks that link to the same target AssertionError: expected { …(10) } to deeply equal { …(14) }

  • Expected

  • Received

    { "___netlify-bootstrap.mjs": false, "___netlify-entry-point.mjs": false, "___netlify-telemetry.mjs": false, "function.mjs": false,

❯ tests/symlinked_included_files.test.ts:109:59 107| 108| // Test to be sure we've made both symlinks, not just one of them 109| expect(await readDirWithType(join(tmpDir, 'function'))).toEqual({ | ^ 110| '___netlify-bootstrap.mjs': false, 111| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/3]⎯

FAIL tests/symlinked_included_files.test.ts > symlinks in subdir of includedFiles are copied over successfully AssertionError: expected { …(5) } to deeply equal { …(6) }

  • Expected

  • Received

    { "___netlify-bootstrap.mjs": false, "___netlify-entry-point.mjs": false, "___netlify-telemetry.mjs": false, "function.cjs": false,

  • "subproject/node_modules/.bin/cli.js": true, "subproject/node_modules/tool/cli.js": false, }

❯ tests/symlinked_included_files.test.ts:153:59 151| }) 152| 153| expect(await readDirWithType(join(tmpDir, 'function'))).toEqual({ | ^ 154| '___netlify-bootstrap.mjs': false, 155| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/3]⎯

</details>

benmccann avatar Jun 07 '25 16:06 benmccann

@mrstork I've implemented the globstar option in tinyglobby, but the CI is still failing because it turns out there's a bug in tinyglobby: https://github.com/SuperchupuDev/tinyglobby/issues/133. It's not the easiest bug to fix because it likely requires new functionality to be implemented in the underlying fdir library. Before I do that, I wanted to check with you to understand exactly what this code is doing and ensure there's not an easier solution

Are you familiar with included_files.ts and what it's doing with regards to symlinks? It looks like it's collecting the links, but not traversing into them - is this necessary and how does it fit into what this package is doing? I don't have a lot of familiarity with this package, so any background information you can share as part of the explanation would be helpful.

benmccann avatar Jun 25 '25 22:06 benmccann

@benmccann All of this predates me by quite a bit, but I'll do my best.

At a high level though, zip-it-and-ship-it (or zisi) is responsible for packaging the necessary pieces of a user's codebase for use in Netlify Functions, or more specifically AWS Lambdas. For more context, I recommend reading the project README, reading through the comments in the code paths you're editing, and reading through the failing tests (as well as the comments in those tests to understand why they are there).

From what I know, none of this code is extraneous. If a test is failing, it's necessary for it to pass in order for the code changes to be merged. As time/capacity allows, I can look to dig in here with you to try to come up with an alternative solutions.

mrstork avatar Jul 03 '25 19:07 mrstork