-
-
Notifications
You must be signed in to change notification settings - Fork 831
chore(deps): switch to tinyglobby #7180
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Drop Yarn v1 and use npm instead * Git add * Fixes * Lets go * Apply overrides properly * Lets go * Fix integrity test * Lets go * Simplify
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…n#7458) Bumps the actions-deps group with 3 updates in the / directory: [@graphql-tools/executor-http](https://github.com/graphql-hive/gateway/tree/HEAD/packages/executors/http), [@graphql-tools/executor-graphql-ws](https://github.com/graphql-hive/gateway/tree/HEAD/packages/executors/graphql-ws) and [@img/sharp-libvips-linux-x64](https://github.com/lovell/sharp-libvips/tree/HEAD/npm/linux-x64). Updates `@graphql-tools/executor-http` from 3.0.0 to 3.0.1 - [Release notes](https://github.com/graphql-hive/gateway/releases) - [Changelog](https://github.com/graphql-hive/gateway/blob/main/packages/executors/http/CHANGELOG.md) - [Commits](https://github.com/graphql-hive/gateway/commits/@graphql-tools/[email protected]/packages/executors/http) Updates `@graphql-tools/executor-graphql-ws` from 3.0.0 to 3.0.1 - [Release notes](https://github.com/graphql-hive/gateway/releases) - [Changelog](https://github.com/graphql-hive/gateway/blob/main/packages/executors/graphql-ws/CHANGELOG.md) - [Commits](https://github.com/graphql-hive/gateway/commits/@graphql-tools/[email protected]/packages/executors/graphql-ws) Updates `@img/sharp-libvips-linux-x64` from 1.2.0 to 1.2.1 - [Release notes](https://github.com/lovell/sharp-libvips/releases) - [Commits](https://github.com/lovell/sharp-libvips/commits/v1.2.1/npm/linux-x64) --- updated-dependencies: - dependency-name: "@graphql-tools/executor-http" dependency-version: 3.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-deps - dependency-name: "@graphql-tools/executor-graphql-ws" dependency-version: 3.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-deps - dependency-name: "@img/sharp-libvips-linux-x64" dependency-version: 1.2.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ardatan#7458)" (ardatan#7460) This reverts commit 48bc977.
Bumps the actions-deps group with 2 updates: [@graphql-tools/executor-http](https://github.com/graphql-hive/gateway/tree/HEAD/packages/executors/http) and [@graphql-tools/executor-graphql-ws](https://github.com/graphql-hive/gateway/tree/HEAD/packages/executors/graphql-ws). Updates `@graphql-tools/executor-http` from 3.0.0 to 3.0.1 - [Release notes](https://github.com/graphql-hive/gateway/releases) - [Changelog](https://github.com/graphql-hive/gateway/blob/main/packages/executors/http/CHANGELOG.md) - [Commits](https://github.com/graphql-hive/gateway/commits/@graphql-tools/[email protected]/packages/executors/http) Updates `@graphql-tools/executor-graphql-ws` from 3.0.0 to 3.0.1 - [Release notes](https://github.com/graphql-hive/gateway/releases) - [Changelog](https://github.com/graphql-hive/gateway/blob/main/packages/executors/graphql-ws/CHANGELOG.md) - [Commits](https://github.com/graphql-hive/gateway/commits/@graphql-tools/[email protected]/packages/executors/graphql-ws) --- updated-dependencies: - dependency-name: "@graphql-tools/executor-http" dependency-version: 3.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-deps - dependency-name: "@graphql-tools/executor-graphql-ws" dependency-version: 3.0.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
The other PR was closed, so I'm reopening this one |
6fb5105
to
c6002c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/loaders/code-file/package.json (1)
56-56
: Dependency switch to tinyglobby looks good; consider aligning versions repo-wide.Other packages use "^0.2.13"; this one uses "^0.2.15". Semver will likely dedupe to 0.2.15, but aligning ranges improves consistency.
Would you like me to scan the repo and generate a one-liner to align all tinyglobby ranges?
packages/loaders/graphql-file/src/index.ts (4)
4-6
: Prefer importingglob
without alias for consistency.Tinyglobby’s docs use
glob
/globSync
. Using those names improves readability across the codebase.-import type { GlobOptions } from 'tinyglobby'; -import { globSync, glob as tinyglobby } from 'tinyglobby'; +import type { GlobOptions } from 'tinyglobby'; +import { glob, globSync } from 'tinyglobby';Then update call sites accordingly (see comments below). (github.com)
49-51
: Option precedence:absolute
can be overridden—confirm intent; also consider renaming helper.
- As written,
{ absolute: true, ...options }
allows callers to setabsolute: false
. If you intend to always return absolute paths, putabsolute: true
last.- The helper name still says “Globby”; consider
createGlobOptions
to avoid confusion post-migration.-function createGlobbyOptions(options: GraphQLFileLoaderOptions): GlobOptions { - return { absolute: true, ...options, ignore: [] }; -} +function createGlobOptions(options: GraphQLFileLoaderOptions): GlobOptions { + // Absolute last to enforce absolute paths; ignore cleared because we pass negated patterns in `globs`. + return { ...options, ignore: [], absolute: true }; +}Note: tinyglobby supports both negated patterns (
'!foo'
) and theignore
option; clearingignore
is fine since you already bake ignores into the patterns. (github.com, npmjs.com)
118-126
: Update comment to reflect tinyglobby.The inline note still says “globby”.
- return [glob]; // bypass globby when no glob character, can be loaded, no ignores and source not requested. Fixes problem with pkg and passes ci tests + return [glob]; // bypass globbing call when no glob character, can be loaded, no ignores and source not requested. Fixes problem with pkg and passes CI tests
131-139
: Update comment to reflect tinyglobby (duplicate in sync path).- return [glob]; // bypass globby when no glob character, can be loaded, no ignores and source not requested. Fixes problem with pkg and passes ci tests + return [glob]; // bypass globbing call when no glob character, can be loaded, no ignores and source not requested. Fixes problem with pkg and passes CI tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.json
(2 hunks)packages/load-files/package.json
(1 hunks)packages/load-files/src/index.ts
(4 hunks)packages/load/tests/loaders/documents/documents-from-glob.spec.ts
(3 hunks)packages/loaders/code-file/package.json
(1 hunks)packages/loaders/code-file/src/index.ts
(3 hunks)packages/loaders/graphql-file/package.json
(1 hunks)packages/loaders/graphql-file/src/index.ts
(4 hunks)packages/loaders/json-file/package.json
(1 hunks)packages/loaders/json-file/src/index.ts
(3 hunks)scripts/build-api-docs.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/loaders/graphql-file/package.json
- package.json
- scripts/build-api-docs.ts
- packages/loaders/json-file/package.json
- packages/load/tests/loaders/documents/documents-from-glob.spec.ts
- packages/loaders/code-file/src/index.ts
- packages/load-files/package.json
- packages/loaders/json-file/src/index.ts
- packages/load-files/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (2)
packages/loaders/graphql-file/src/index.ts (2)
127-129
: Async globbing call migration LGTM.If you applied the earlier import/name tweaks, also update this line:
- const result = await tinyglobby(globs, createGlobbyOptions(options)); + const result = await glob(globs, createGlobOptions(options));
140-142
: Sync globbing call migration LGTM.If you applied the earlier import/name tweaks, also update this line:
- const result = globSync(globs, createGlobbyOptions(options)); + const result = globSync(globs, createGlobOptions(options));
c6002c5
to
7610965
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/loaders/json-file/package.json (1)
55-55
: Bump tinyglobby to ^0.2.15 in json-file and load-files packages
Update both packages/loaders/json-file/package.json (line 55) and packages/load-files/package.json (line 54) from"tinyglobby": "^0.2.13"
to"tinyglobby": "^0.2.15"
so all packages use the same version and enable npm/yarn/pnpm dedupe.packages/load/tests/loaders/documents/documents-from-glob.spec.ts (2)
23-24
: Stabilize path comparison by using absolute paths from globSyncEnsure expectedFiles are absolute to match loader output and avoid platform-specific path issues.
Apply this diff:
- const expectedFiles = globSync(glob); + const expectedFiles = globSync(glob, { absolute: true });
41-42
: Repeat absolute paths for the multi-file caseMirror the change above for consistency.
Apply this diff:
- const expectedFiles = globSync(glob); + const expectedFiles = globSync(glob, { absolute: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.json
(2 hunks)packages/load-files/package.json
(1 hunks)packages/load-files/src/index.ts
(6 hunks)packages/load/tests/loaders/documents/documents-from-glob.spec.ts
(3 hunks)packages/loaders/code-file/package.json
(1 hunks)packages/loaders/code-file/src/index.ts
(3 hunks)packages/loaders/graphql-file/package.json
(1 hunks)packages/loaders/graphql-file/src/index.ts
(4 hunks)packages/loaders/json-file/package.json
(1 hunks)packages/loaders/json-file/src/index.ts
(3 hunks)scripts/build-api-docs.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- scripts/build-api-docs.ts
- packages/load-files/package.json
- packages/loaders/code-file/package.json
- packages/loaders/graphql-file/package.json
- packages/loaders/code-file/src/index.ts
- packages/loaders/json-file/src/index.ts
- packages/loaders/graphql-file/src/index.ts
🧰 Additional context used
🪛 GitHub Check: Unit Test on Bun
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Type Check on GraphQL v15
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
🪛 GitHub Check: Full Check on GraphQL v16
packages/load-files/src/index.ts
[failure] 157-157:
Cannot invoke an object which is possibly 'undefined'.
[failure] 157-157:
This expression is not callable.
[failure] 261-261:
Cannot invoke an object which is possibly 'undefined'.
[failure] 261-261:
This expression is not callable.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (4)
packages/load/tests/loaders/documents/documents-from-glob.spec.ts (1)
9-9
: LGTM: migrated to tinyglobby’s globSyncpackages/load-files/src/index.ts (3)
5-5
: LGTM: adding fileURLToPath for URL cwd support
7-7
: LGTM: migrate to tinyglobby API (GlobOptions, globSync, glob)
102-104
: Docs note: now refers to tinyglobbyComment update looks correct.
f6030ce
to
daeb5f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/load-files/src/index.ts (3)
5-5
: Fix unused import (build is red) by actually normalizing URL cwdTS6133: fileURLToPath is imported but unused. Use it to normalize GlobOptions.cwd.
Apply together with the diffs below (scanForFilesSync/scanForFiles).
67-69
: Normalize URL cwd and force absolute last (also fixes the unused import)Ensures consistent absolute paths and supports URL cwd.
-function scanForFilesSync(globStr: string | string[], globOptions: GlobOptions = {}): string[] { - return globSync(globStr, { absolute: true, ...globOptions }); -} +function scanForFilesSync(globStr: string | string[], globOptions: GlobOptions = {}): string[] { + const normalizedCwd = + globOptions?.cwd instanceof URL ? fileURLToPath(globOptions.cwd) : globOptions?.cwd; + return globSync(globStr, { ...globOptions, cwd: normalizedCwd, absolute: true }); +}
187-190
: Mirror the sync fix in async pathSame normalization and absolute ordering.
-async function scanForFiles( - globStr: string | string[], - globOptions: GlobOptions = {}, -): Promise<string[]> { - return tinyglobby(globStr, { absolute: true, ...globOptions }); -} +async function scanForFiles( + globStr: string | string[], + globOptions: GlobOptions = {}, +): Promise<string[]> { + const normalizedCwd = + globOptions?.cwd instanceof URL ? fileURLToPath(globOptions.cwd) : globOptions?.cwd; + return tinyglobby(globStr, { ...globOptions, cwd: normalizedCwd, absolute: true }); +}
🧹 Nitpick comments (3)
packages/loaders/graphql-file/src/index.ts (1)
49-51
: Ensure absolute paths cannot be overriddenPlace absolute: true last so user options can’t flip it back to false.
-function createGlobbyOptions(options: GraphQLFileLoaderOptions): GlobOptions { - return { absolute: true, ...options, ignore: [] }; -} +function createGlobbyOptions(options: GraphQLFileLoaderOptions): GlobOptions { + return { ...options, ignore: [], absolute: true }; +}packages/load-files/src/index.ts (2)
102-104
: Docstring reflects tinyglobby; consider clarifying cwd typeSince cwd may be string or URL, note that here to guide consumers.
5-5
: Normalize cwd for createRequire join (outside diff for awareness)If options.globOptions.cwd is a URL, join() will mis-handle it. Normalize first.
Outside this hunk, in both sync and async loaders:
// before creating requireMethod / cwdRequire const globCwd = options?.globOptions?.cwd; const normalizedCwd = globCwd instanceof URL ? fileURLToPath(globCwd) : globCwd; // sync: const requireMethod = execOptions.requireMethod || createRequire(join(normalizedCwd || cwd(), 'noop.js')); // async: const cwdRequire = createRequire(join(normalizedCwd || cwd(), 'noop.js'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.json
(1 hunks)packages/load-files/package.json
(1 hunks)packages/load-files/src/index.ts
(4 hunks)packages/load/tests/loaders/documents/documents-from-glob.spec.ts
(3 hunks)packages/loaders/code-file/package.json
(1 hunks)packages/loaders/code-file/src/index.ts
(3 hunks)packages/loaders/graphql-file/package.json
(1 hunks)packages/loaders/graphql-file/src/index.ts
(4 hunks)packages/loaders/json-file/package.json
(1 hunks)packages/loaders/json-file/src/index.ts
(3 hunks)scripts/build-api-docs.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/build-api-docs.ts
- packages/loaders/code-file/package.json
- packages/load-files/package.json
- packages/load/tests/loaders/documents/documents-from-glob.spec.ts
- packages/loaders/json-file/src/index.ts
- packages/loaders/code-file/src/index.ts
🧰 Additional context used
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Unit Test on Bun
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Type Check on GraphQL v15
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Full Check on GraphQL v16
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 5-5:
'fileURLToPath' is declared but its value is never read.
🪛 GitHub Actions: Tests
packages/load-files/src/index.ts
[error] 5-5: TypeScript error TS6133: 'fileURLToPath' is declared but its value is never read.
🔇 Additional comments (7)
package.json (1)
85-85
: Use caret range for tinyglobby in root devDepsChange the root devDependency to ^0.2.14 to help dedupe across workspaces.
- "tinyglobby": "0.2.14", + "tinyglobby": "^0.2.14",Verification notes: rg only found comment mentions of "globby" (packages/loaders/graphql-file/src/index.ts:125,138); package-lock.json still references globby (multiple versions). npm ls returned no installed workspace packages. After updating, run npm install and then:
npm ls tinyglobby globby --workspaces --allpackages/loaders/graphql-file/package.json (1)
56-56
: Dependency switch looks goodPackage now depends on tinyglobby. No issues spotted.
packages/loaders/json-file/package.json (1)
55-55
: Dependency switch looks goodMirrors other loaders; consistent with the PR intent.
packages/loaders/graphql-file/src/index.ts (3)
4-6
: Import switch to tinyglobby is correctTypes and symbols align with usage below.
127-128
: Async globbing swap LGTMtinyglobby(...) replaces globby(...) as intended.
140-141
: Sync globbing swap LGTMglobSync(...) usage is correct.
packages/load-files/src/index.ts (1)
7-7
: Import switch to tinyglobby is correctTypes and symbols match usage.
daeb5f0
to
d40d595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/load-files/src/index.ts (2)
151-155
: Normalize URL cwd before createRequire join.If
options.globOptions.cwd
is a URL,path.join
receives a non-string. Normalize first.- const requireMethod = - execOptions.requireMethod || createRequire(join(options?.globOptions?.cwd || cwd(), 'noop.js')); + const userCwd = options?.globOptions?.cwd; + const normalizedCwd = userCwd instanceof URL ? fileURLToPath(userCwd) : userCwd; + const requireMethod = + execOptions.requireMethod || createRequire(join(normalizedCwd || cwd(), 'noop.js'));
254-257
: Async import fallback: normalize URL cwd before createRequire.- const cwdRequire = createRequire(join(options?.globOptions?.cwd || cwd(), 'noop.js')); + const userCwd = options?.globOptions?.cwd; + const normalizedCwd = userCwd instanceof URL ? fileURLToPath(userCwd) : userCwd; + const cwdRequire = createRequire(join(normalizedCwd || cwd(), 'noop.js'));
♻️ Duplicate comments (3)
packages/load-files/src/index.ts (3)
137-149
: Preserve merged defaults: pass execOptions.globOptions to the scanner.Passing
options.globOptions
drops defaults fromLoadFilesDefaultOptions
(e.g.,absolute
, defaultcwd
).- const relevantPaths = scanForFilesSync( + const relevantPaths = scanForFilesSync( asArray(pattern).map(path => isDirectorySync(path) ? buildGlob( unixify(path), execOptions.extensions, execOptions.ignoredExtensions, execOptions.recursive, ) : unixify(path), ), - options.globOptions, + execOptions.globOptions, );
248-249
: Also pass execOptions.globOptions in async path.- options.globOptions, + execOptions.globOptions,
186-189
: Async scanner: same fixes — absolute last + URL cwd normalization.Mirror the sync improvements here.
-async function scanForFiles( - globStr: string | string[], - globOptions: GlobOptions = {}, -): Promise<string[]> { - return tinyglobby(globStr, { absolute: true, ...globOptions }); -} +async function scanForFiles( + globStr: string | string[], + globOptions: GlobOptions = {}, +): Promise<string[]> { + const normalizedCwd = + globOptions?.cwd instanceof URL ? fileURLToPath(globOptions.cwd) : globOptions?.cwd; + return tinyglobby(globStr, { ...globOptions, cwd: normalizedCwd, absolute: true }); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.json
(1 hunks)packages/load-files/package.json
(1 hunks)packages/load-files/src/index.ts
(4 hunks)packages/load/tests/loaders/documents/documents-from-glob.spec.ts
(3 hunks)packages/loaders/code-file/package.json
(1 hunks)packages/loaders/code-file/src/index.ts
(3 hunks)packages/loaders/graphql-file/package.json
(1 hunks)packages/loaders/graphql-file/src/index.ts
(4 hunks)packages/loaders/json-file/package.json
(1 hunks)packages/loaders/json-file/src/index.ts
(3 hunks)scripts/build-api-docs.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- package.json
- packages/load-files/package.json
- scripts/build-api-docs.ts
- packages/loaders/graphql-file/package.json
- packages/load/tests/loaders/documents/documents-from-glob.spec.ts
- packages/loaders/json-file/package.json
- packages/loaders/code-file/package.json
- packages/loaders/graphql-file/src/index.ts
- packages/loaders/code-file/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Bun
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
🔇 Additional comments (5)
packages/load-files/src/index.ts (2)
6-6
: Import aliasing looks good.Switch to tinyglobby + aliasing
glob as tinyglobby
is clear and consistent.
101-102
: Docs update matches implementation.Updating to “tinyglobby” and
GlobOptions
is accurate.packages/loaders/json-file/src/index.ts (3)
4-5
: Import changes look correct.Using tinyglobby types and functions aligns with the migration.
93-94
: LGTM.Async glob resolution correctly uses tinyglobby with options helper.
99-100
: LGTM.Sync glob resolution matches async implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/load-files/src/index.ts (1)
189-192
: Async scanner: normalize cwd and prevent overriding absolute (parity with sync)Currently
{ absolute: true, ...globOptions }
allows callers to overrideabsolute
. Normalizecwd
like sync and setabsolute
last.async function scanForFiles( globStr: string | string[], globOptions: GlobOptions = {}, ): Promise<string[]> { - return tinyglobby(globStr, { absolute: true, ...globOptions }); + const rawCwd: unknown = (globOptions as any)?.cwd; + const normalizedCwd: string | undefined = + rawCwd instanceof URL ? fileURLToPath(rawCwd) : typeof rawCwd === 'string' ? rawCwd : undefined; + return tinyglobby(globStr, { ...(globOptions || {}), cwd: normalizedCwd, absolute: true }); }
🧹 Nitpick comments (1)
packages/load-files/src/index.ts (1)
104-105
: Consider widening the public type if URL cwd is intendedIf callers should be able to pass
cwd
as aURL
, reflect it in the type; otherwise, drop URL normalization for consistency.- // Additional options to pass to tinyglobby - globOptions?: GlobOptions; + // Additional options to pass to tinyglobby + // Allow URL for cwd to match runtime normalization + globOptions?: Omit<GlobOptions, 'cwd'> & { cwd?: string | URL };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/load-files/src/index.ts
(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Full Check on GraphQL v16
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Unit Test on Node 24 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Unit Test on Bun
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
🪛 GitHub Check: Type Check on GraphQL v15
packages/load-files/src/index.ts
[failure] 69-69:
The left-hand side of an 'instanceof' expression must be of type 'any', an object type or a type parameter.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (2)
packages/load-files/src/index.ts (2)
5-5
: Import looks goodNeeded for URL→path normalization; keep as-is.
7-7
: Dependency swap is correcttinyglobby import/aliases are appropriate.
d40d595
to
f7d1bce
Compare
f7d1bce
to
498841b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/loaders/graphql-file/src/index.ts (1)
49-51
: Prevent leaking non-glob loader props into tinyglobby
options
includes loader-specific fields (e.g.,skipGraphQLImport
,pathAliases
) that tinyglobby ignores. To keep types clean and future-proof, assert or narrow toGlobOptions
while preserving the override ofignore
:-function createGlobbyOptions(options: GraphQLFileLoaderOptions): GlobOptions { - return { absolute: true, ...options, ignore: [] }; -} +function createGlobbyOptions(options: GraphQLFileLoaderOptions): GlobOptions { + return { absolute: true, ...(options as GlobOptions), ignore: [] }; +}packages/loaders/code-file/src/index.ts (3)
62-64
: Keep tinyglobby option typing tightMirror the approach from the GraphQL loader to avoid passing loader-only props:
-function createGlobbyOptions(options: CodeFileLoaderOptions): GlobOptions { - return { absolute: true, ...options, ignore: [] }; -} +function createGlobbyOptions(options: CodeFileLoaderOptions): GlobOptions { + return { absolute: true, ...(options as GlobOptions), ignore: [] }; +}
141-142
: Optional: short-circuit when no glob charsGraphQL loader bypasses globbing for plain file paths; doing the same here can avoid unnecessary I/O and matches prior optimization.
async resolveGlobs(glob: string, options: CodeFileLoaderOptions) { options = this.getMergedOptions(options); - const globs = this._buildGlobs(glob, options); - return tinyglobby(globs, createGlobbyOptions(options)); + if (!glob.includes('*') && this.canLoadSync(glob, options) && !asArray(options.ignore || []).length) { + return [glob]; + } + const globs = this._buildGlobs(glob, options); + return tinyglobby(globs, createGlobbyOptions(options)); }
147-148
: Optional: add the same short-circuit in sync pathresolveGlobsSync(glob: string, options: CodeFileLoaderOptions) { options = this.getMergedOptions(options); - const globs = this._buildGlobs(glob, options); - return globSync(globs, createGlobbyOptions(options)); + if (!glob.includes('*') && this.canLoadSync(glob, options) && !asArray(options.ignore || []).length) { + return [glob]; + } + const globs = this._buildGlobs(glob, options); + return globSync(globs, createGlobbyOptions(options)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.json
(1 hunks)packages/load-files/package.json
(1 hunks)packages/load-files/src/index.ts
(4 hunks)packages/load/tests/loaders/documents/documents-from-glob.spec.ts
(3 hunks)packages/loaders/code-file/package.json
(1 hunks)packages/loaders/code-file/src/index.ts
(3 hunks)packages/loaders/graphql-file/package.json
(1 hunks)packages/loaders/graphql-file/src/index.ts
(4 hunks)packages/loaders/json-file/package.json
(1 hunks)packages/loaders/json-file/src/index.ts
(3 hunks)scripts/build-api-docs.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/load-files/package.json
- packages/load/tests/loaders/documents/documents-from-glob.spec.ts
- packages/loaders/code-file/package.json
- scripts/build-api-docs.ts
- packages/loaders/json-file/package.json
- package.json
- packages/load-files/src/index.ts
- packages/loaders/json-file/src/index.ts
- packages/loaders/graphql-file/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (4)
packages/loaders/graphql-file/src/index.ts (3)
4-5
: Switch to tinyglobby imports — OKAlias avoids name collision with the
glob
parameter in methods; looks good.
140-141
: Sync globbing replacement — OK
globSync
drop-in looks correct; options merge maintains previous behavior.
127-127
: Parity check: confirm tinyglobby negative-pattern & absolute-path parity
- Check packages/loaders/graphql-file/src/index.ts:127 (tinyglobby(globs, createGlobbyOptions(options))).
- Verify tinyglobby yields identical matches to globby for patterns with leading
!
(add a small repro/test comparing outputs).- Verify results are absolute on Windows (drive-letter preserved) with our unixify/drive-letter handling; ensure createGlobbyOptions doesn't change that.
- Confirm no remaining
globby
imports/usages in the repo (search forfrom 'globby'
,GlobbyOptions
, orglobby.sync
) and fix if found.packages/loaders/code-file/src/index.ts (1)
6-8
: Imports migrated to tinyglobby — OKAlias avoids collisions with local glob parameter names; no issues spotted. Your search returned no output; unable to confirm absence of any remaining 'globby' references. Re-run and paste the output of:
rg -nP "(from\s+['"]globby['"]|\bGlobbyOptions\b|globby\.sync\b)" -S
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
http://npmgraph.js.org/?q=globby - 23 dependencies
http://npmgraph.js.org/?q=tinyglobby - 2 dependencies
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Existing test suite
Checklist:
CONTRIBUTING doc and the
style guidelines of this project