Dependency cleanup #6896
Dependency cleanup #6896
Conversation
🦋 Changeset detectedLatest commit: d7c2fed The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
it only fails on windows, i wonder why |
| const _nodes = nodes as ASTNode[]; | ||
| return cacheResult( | ||
| _nodes.sort((nodeA, nodeB) => { | ||
| const kindComparison = nodeA.kind.localeCompare(nodeB.kind); | ||
| if (kindComparison !== 0) return kindComparison; | ||
|
|
||
| const nameA = (nodeA as any).name?.value ?? ''; | ||
| const nameB = (nodeB as any).name?.value ?? ''; | ||
|
|
||
| return cacheResult(sortBy(nodes as any, 'kind', 'name.value')); | ||
| return nameA.localeCompare(nameB); | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
do I need to add comments here or it's fine as it is?
|
blocked by a performance regression in tinyglobby on windows |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update several GraphQL tools packages by replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FL as FileLoader Function
participant TG as tinyglobby
participant UT as Utils (unixifyWithDriveLetter)
U ->> FL: Invoke file loading function
FL ->> TG: Call glob or globSync with file pattern
TG -->> FL: Return list of file paths
FL ->> UT: Normalize each path using unixifyWithDriveLetter
UT -->> FL: Return normalized path
FL -->> U: Return processed file list
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/executor/src/execution/__tests__/abort-signal.test.tsOops! Something went wrong! :( ESLint: 9.31.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/executor/src/execution/__tests__/error-handling.test.tsOops! Something went wrong! :( ESLint: 9.31.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by babel.config.jsOops! Something went wrong! :( ESLint: 9.31.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/utils/src/unixifyWithDriveLetter.ts (1)
4-4: Consider supporting lowercase drive letters.The regex
^[A-Z]:\\only matches uppercase drive letters. Windows drive letters are case-insensitive, so consider using^[A-Za-z]:\\to support both uppercase and lowercase drive letters.- if (path.match(/^[A-Z]:\\/)) { + if (path.match(/^[A-Za-z]:\\/)) {packages/documents/src/sort-executable-nodes.ts (2)
46-46: Consider optimizing string comparisons.Using
localeComparefor simple ASCII string comparisons might be unnecessarily expensive. For better performance, consider using the<operator when you know the strings only contain ASCII characters.- return cacheResult(nodes.sort((a, b) => a.name.value.localeCompare(b.name.value))); + return cacheResult(nodes.sort((a, b) => (a.name.value < b.name.value ? -1 : a.name.value > b.name.value ? 1 : 0)));Also applies to: 50-52, 56-56
63-79: Add comments explaining the sorting strategy.The sorting logic for SelectionNode uses prefixes and a complex getSortKey function. Consider adding comments to explain the sorting strategy and why certain prefixes are chosen.
Based on the past review comment asking about documentation needs, I recommend adding the following comments:
+ // Sort nodes by type (field, fragment spread, inline fragment) and then by name + // Using string prefixes to ensure consistent ordering: + // - '0' for fields (appearing first) + // - '1' for fragment spreads (appearing second) + // - '2' for inline fragments (appearing last) nodes.sort((a, b) => { const getSortKey = (node: SelectionNode) => {packages/loaders/json-file/src/index.ts (1)
94-102: Consider adding error handling for glob operations.The glob operations might fail due to various reasons (permissions, invalid patterns). Consider adding try-catch blocks to handle potential errors gracefully.
async resolveGlobs(path: string, options: JsonFileLoaderOptions) { const globs = this._buildGlobs(path, options); - const result = await glob(globs, createGlobbyOptions(options)); - return result; + try { + const result = await glob(globs, createGlobbyOptions(options)); + return result; + } catch (error) { + throw new Error(`Failed to resolve glob patterns: ${error.message}`); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.changeset/nasty-peas-ring.md(1 hunks)package.json(1 hunks)packages/documents/package.json(1 hunks)packages/documents/src/sort-executable-nodes.ts(1 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(4 hunks)packages/loaders/graphql-file/package.json(1 hunks)packages/loaders/graphql-file/src/index.ts(5 hunks)packages/loaders/json-file/package.json(1 hunks)packages/loaders/json-file/src/index.ts(2 hunks)packages/utils/package.json(1 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/unixifyWithDriveLetter.ts(1 hunks)scripts/build-api-docs.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/nasty-peas-ring.md
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 20 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 20 (ubuntu-latest) and GraphQL v15
- GitHub Check: Type Check on GraphQL v15
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
🔇 Additional comments (20)
packages/utils/src/index.ts (1)
62-62: LGTM!The export statement is correctly placed and properly exposes the new utility function.
packages/loaders/json-file/src/index.ts (1)
87-90: Verify Windows compatibility with tinyglobby.Given the reported test failures on Windows and the switch to tinyglobby, ensure that the glob pattern handling works correctly on Windows systems.
Let's verify the glob patterns used in tests:
scripts/build-api-docs.ts (2)
39-39: LGTM! Consistent styling replacement.The replacement of
chalkwithnode:util.styleTextis done consistently across the file.Also applies to: 83-83
3-4: Verify Windows compatibility for file globbing.The
globSyncusage might need path normalization for Windows paths. Consider normalizing the workspace paths before passing them toglobSync.Also applies to: 16-16
✅ Verification successful
Windows Compatibility Check Verified: No Normalization Needed
The package workspaces in package.json use forward slashes (e.g. "packages/", "packages/loaders/", etc.), so there are no Windows-style paths triggering issues with globSync. No normalization is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if workspace paths in package.json contain Windows-style paths # that might need normalization before passing to globSync jq -r '.workspaces[]' package.json | while read -r pattern; do if [[ $pattern == *"\\"* ]]; then echo "Found Windows-style path: $pattern" fi doneLength of output: 249
packages/loaders/graphql-file/src/index.ts (3)
4-4: LGTM! Consistent import changes.The imports are correctly updated to use
tinyglobbyandunixifyWithDriveLetterfrom utils.Also applies to: 13-13
104-113: LGTM! Consistent glob handling.The changes to use
globandglobSyncfromtinyglobbyare implemented correctly, with proper options handling.Also applies to: 126-126
35-35: Verify Windows path handling.The usage of
unixifyWithDriveLetterfor path normalization is correct, but we should verify it handles Windows paths properly, especially in the_buildGlobsmethod.Also applies to: 98-99
✅ Verification successful
Verified Windows Path Handling in GraphQL File Loader
- The source file (
packages/loaders/graphql-file/src/index.ts) usesunixifyWithDriveLetterconsistently—including within the_buildGlobsmethod—to normalize paths.- Although no tests explicitly include Windows-style paths (i.e. containing "C:\"), the integration of the normalization function in key locations (lines 13, 98, and 99) confirms that the intended Windows path handling is correctly applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the codebase has tests with Windows-style paths # that would validate unixifyWithDriveLetter handling rg -l 'C:\\' packages/loaders/graphql-file/testsLength of output: 50
Script:
#!/bin/bash # Verify usage of unixifyWithDriveLetter and related _buildGlobs function in packages/loaders/graphql-file/src/index.ts echo "Searching for unixifyWithDriveLetter in GraphQL File Loader..." rg -n "unixifyWithDriveLetter" packages/loaders/graphql-file/src/index.ts echo "Searching for _buildGlobs in GraphQL File Loader..." rg -n "_buildGlobs" packages/loaders/graphql-file/src/index.tsLength of output: 712
packages/load/tests/loaders/documents/documents-from-glob.spec.ts (1)
9-9: LGTM! Proper path normalization for tests.The explicit path normalization with
.replace(/\\/g, '/')is a good practice for ensuring consistent behavior across platforms.Also applies to: 23-23, 41-41
packages/load-files/src/index.ts (3)
6-7: LGTM! Clean import changes.The imports are correctly updated to use
tinyglobbyandunixifyWithDriveLetter.
68-69: LGTM! Consistent type changes.The type changes from
GlobbyOptionstoGlobOptionsare properly propagated through the interface and function signatures.Also applies to: 104-104
143-148: Verify Windows path handling in file loading.The usage of
unixifyWithDriveLetterfor path normalization looks correct, but we should verify it handles Windows paths properly in both sync and async file loading paths.Also applies to: 242-247
✅ Verification successful
I'll generate a follow-up script to check the definition of
unixifyWithDriveLetterin the repository. This will help confirm whether its implementation properly normalizes Windows paths. Please run the script below and share its output.
Windows Path Handling Verified in File Loading
The usage of
unixifyWithDriveLetteris consistent in both sync and async file loading code paths, and its definition (located inpackages/utils/src/unixifyWithDriveLetter.ts) suggests that Windows paths are normalized uniformly. No evidence was found that indicates discrepancies or issues with the Windows path normalization in either case.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are Windows-specific test cases # that validate the path handling in loadFiles and loadFilesSync # Look for Windows path tests rg -A 5 'C:\\|\\\\' packages/load-files/tests # Look for path normalization tests rg -A 5 'path.normalize|path.sep' packages/load-files/testsLength of output: 109
Script:
#!/bin/bash # Search for the definition of unixifyWithDriveLetter in the entire repository. rg -n "function unixifyWithDriveLetter" .Length of output: 155
packages/loaders/code-file/src/index.ts (3)
6-6: LGTM! Import changes align with dependency updates.The changes correctly replace
globbywithtinyglobbyand addunixifyWithDriveLetterfrom@graphql-tools/utils.Also applies to: 20-20
62-62: LGTM! Function signature updated for tinyglobby.The type change from
GlobbyOptionstoGlobOptionscorrectly reflects the new package's type system.
134-137: Verify Windows compatibility with unixifyWithDriveLetter.The changes look good, but given the PR comments about Windows-specific issues, we should verify the path handling.
Also applies to: 141-144, 150-150
✅ Verification successful
Windows compatibility for
unixifyWithDriveLetterappears to be verified.
- The grep for Windows-style paths (
[A-Z]:\) in the test files returned no instances, suggesting that none of the tests are inadvertently using hardcoded Windows paths.- The usage of
unixifyWithDriveLetterin the target file (as well as on lines 141–144 and 150) remains consistent with the intent to normalize paths across environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any Windows-specific paths in test files that might be affected # Look for Windows-style paths in test files rg -l '(?i)[A-Z]:\\' --type ts --type js # Look for path-related test cases rg -A 5 'test.*path' --type tsLength of output: 6568
packages/utils/package.json (1)
43-44: LGTM! Dependencies updated correctly.The addition of
unixifyaligns with the new path handling requirements.packages/load-files/package.json (1)
54-56: LGTM! Dependencies updated consistently.The changes align with the PR objective:
- Replaced
globbywithtinyglobby- Added
@graphql-tools/utilsfor path handling utilitiespackages/loaders/json-file/package.json (1)
55-56: Dependency Update: Correct Replacement of globby with tinyglobby.
The new dependency"tinyglobby": "^0.2.10"is correctly added, which aligns with the overall objective of reducing the dependency footprint. Make sure that any code usage previously tied toglobbyhas been updated accordingly.packages/loaders/code-file/package.json (1)
56-57: Dependency Update: Successful Transition to tinyglobby.
The update replacingglobbywith"tinyglobby": "^0.2.10"is applied correctly in the dependency block. This change is consistent with the dependency cleanup initiative.packages/loaders/graphql-file/package.json (1)
56-59: Dependency Consistency: Revisit the Usage of 'unixify'.
While"globby": "^11.0.3"has been replaced with"tinyglobby": "^0.2.10", the dependency"unixify": "^1.0.0"remains in the list. Given that other packages in this cleanup have removedunixifyin favor of native functionality, please verify if this dependency is still required for the@graphql-tools/graphql-file-loaderpackage. If not, consider removing it to maintain consistency across the repository.package.json (1)
78-78: Project Dependency Update: Cleanup of Redundant Packages.
The rootpackage.jsonreflects the removal of"chalk": "5.4.1"and"globby": "11.1.0", and the addition of"tinyglobby": "^0.2.10"within thedevDependencies. This aligns well with the dependency cleanup objectives. Ensure that any tooling or scripts that depended on the removed packages are updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/load-files/src/index.ts (2)
141-146: Consider adding path normalization tests.The consistent use of
unixifyWithDriveLetteris good, but given the Windows test failures, we should ensure proper test coverage for path normalization.Would you like me to help create test cases that verify path normalization works correctly across different platforms? This could help identify the root cause of the Windows test failures.
Also applies to: 240-245
66-68: Implement cross-platform testing strategy.Given the Windows test failures, consider:
- Adding specific test cases for Windows paths
- Testing with both forward and backward slashes
- Validating path normalization with drive letters
Consider implementing a test helper that validates path handling across different platforms. This could help prevent platform-specific issues in the future.
Also applies to: 185-188, 141-146, 240-245
packages/loaders/code-file/src/index.ts (1)
140-143: Consider moving glob resolution to utils.The glob resolution methods are well-implemented, but as previously suggested, these utility functions could be moved to a separate utils file for better code organization and reusability.
Consider extracting these methods to a new file like
glob-utils.ts:// glob-utils.ts import { glob, globSync, type GlobOptions } from 'tinyglobby'; import { unixifyWithDriveLetter } from '@graphql-tools/utils'; import type { CodeFileLoaderOptions } from './types'; export function createGlobbyOptions(options: CodeFileLoaderOptions): GlobOptions { return { absolute: true, ...options, ignore: [] }; } export function buildGlobs(path: string, options: CodeFileLoaderOptions) { const ignores = asArray(options.ignore || []); return [ unixifyWithDriveLetter(path), ...ignores.map(v => buildIgnoreGlob(unixifyWithDriveLetter(v))), ]; } export async function resolveGlobs(path: string, options: CodeFileLoaderOptions) { return glob(buildGlobs(path, options), createGlobbyOptions(options)); } export function resolveGlobsSync(path: string, options: CodeFileLoaderOptions) { return globSync(buildGlobs(path, options), createGlobbyOptions(options)); }Also applies to: 149-149
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/load-files/src/index.ts(6 hunks)packages/loaders/code-file/src/index.ts(4 hunks)packages/loaders/json-file/src/index.ts(3 hunks)packages/utils/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v16
- GitHub Check: Unit Test on Node 23 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 22 (ubuntu-latest) and GraphQL v15
- GitHub Check: Unit Test on Node 18 (ubuntu-latest) and GraphQL v15
🔇 Additional comments (10)
packages/load-files/src/index.ts (2)
66-68: Validate glob pattern handling across platforms.The switch to
tinyglobbyfunctions looks correct, but we should ensure that the glob patterns work consistently across platforms, especially on Windows where path separators differ.Run the following script to analyze glob patterns:
Also applies to: 185-188
✅ Verification successful
Glob Pattern Validation Across Platforms Confirmed
The glob pattern functions in
packages/load-files/src/index.tscorrectly incorporate Windows path normalization (viaunixifyWithDriveLetter) when building glob patterns. The search output confirms that thebuildGlobfunction is actively used in the file scanning functions and does not reveal any Windows-specific issues. The other matched glob patterns in unrelated files are not indicative of problems within this module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check glob pattern usage # Test: Look for glob patterns that might cause issues on Windows rg -A 2 "buildGlob\(" || echo "No buildGlob calls found" # Check for path separator usage in glob patterns rg -l "[/\\\\]\\*\\*[/\\\\]" || echo "No path separators in glob patterns found"Length of output: 867
6-7: Verify Windows compatibility with the new dependencies.The switch from
globbytotinyglobbyand the use ofunixifyWithDriveLettersuggests an attempt to improve Windows compatibility. However, given the reported test failures on Windows, we should verify the behavior.Run the following script to check Windows path handling:
✅ Verification successful
Windows dependency changes verified.
- The load-files update correctly imports and uses
unixifyWithDriveLetterto handle Windows paths.- Searches in the repository show that Windows path conversions still occur via explicit
.replace(/\\/g, '/')in test files, which aligns with historical behavior.- No conflicting or extra usages of
unixifyWithDriveLetterwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check path handling in the codebase # Test 1: Check for Windows-style paths in tests rg -g '*.{test,spec}.{ts,js}' '\\\\' || echo "No Windows paths in tests" # Test 2: Check usage of path utilities ast-grep --pattern 'unixifyWithDriveLetter($path)' || echo "No other usages found"Length of output: 942
packages/loaders/json-file/src/index.ts (5)
4-4: LGTM! Import changes align with dependency cleanup objectives.The change from
globbytotinyglobbyis correctly implemented with all necessary functions imported.
24-26: LGTM! Type signature updated correctly.The return type change from
GlobbyOptionstoGlobOptionsaligns with the tinyglobby migration.
86-89: Consider extracting glob building logic to utils.The glob pattern building logic could be moved to a shared utility function, as it's likely used across multiple loader packages.
93-101: LGTM! Method signatures and implementations updated correctly.The changes improve clarity by renaming the parameter from
globtopathand correctly implement the tinyglobby API.
12-12: Verify Windows path handling.Given the reported test failures on Windows, let's verify the path handling implementation.
✅ Verification successful
Windows path handling verified.
The implementation inpackages/utils/src/unixifyWithDriveLetter.tscorrectly detects Windows drive-letter paths via the regex and properly transforms the path using the importedunixifyfunction. Usage across loaders confirms consistent application of this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Windows path handling implementation and usage # Check implementation of unixifyWithDriveLetter ast-grep --pattern 'function unixifyWithDriveLetter($_) { $$$ }' # Check test coverage for Windows paths rg -l "test.*windows.*path" -g "*.test.*"Length of output: 112
Script:
#!/bin/bash # Check for any occurrence of 'unixifyWithDriveLetter' in the repository source code rg -n "unixifyWithDriveLetter" # Check for usage in the specific file to see its import and usage rg -n "unixifyWithDriveLetter" packages/loaders/json-file/src/index.ts # Additionally, check for Windows-specific patterns such as drive letters in test files rg -n "C:" --glob "*.test.*"Length of output: 1669
Script:
#!/bin/bash # Print the contents of the unixifyWithDriveLetter implementation to inspect its handling of Windows paths. sed -n '1,150p' packages/utils/src/unixifyWithDriveLetter.tsLength of output: 297
packages/loaders/code-file/src/index.ts (3)
6-6: LGTM! Import changes align with dependency cleanup.The replacement of
globbywithtinyglobbyand addition ofunixifyWithDriveLetterare correctly implemented.Also applies to: 20-20
61-61: Verify Windows compatibility with tinyglobby options.The type change from
GlobbyOptionstoGlobOptionsis correct. However, given the reported Windows-specific test failures, we should verify the glob options compatibility.Let's check if there are any Windows-specific glob patterns or options in the codebase:
✅ Verification successful
Windows compatibility verified for tinyglobby GlobOptions usage.
The search for Windows-specific glob patterns and options yielded no discrepancies or misconfigurations. The usage of
GlobOptionsin our code—in particular in the functioncreateGlobbyOptionsinpackages/loaders/code-file/src/index.ts—is consistent with the repository’s handling of file paths across platforms.
- The conversion to
GlobOptionsis aligned with the switch totinyglobbyand is used consistently across similar modules.- There is no evidence from the search output that Windows-specific patterns or any incompatible options are being employed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for Windows-specific glob patterns and options # Check for Windows path patterns (backslashes, drive letters) rg -i '([a-z]:\\|\\\\|\\[^"])' # Check for glob options that might affect Windows compatibility rg -i '(globOptions|GlobOptions|glob\(.*\{)'Length of output: 49840
133-136: Verify Windows path handling with unixifyWithDriveLetter.The use of
unixifyWithDriveLettershould better handle Windows paths by preserving drive letters. However, we should verify this works as expected.Let's check the implementation and usage of
unixifyWithDriveLetter:✅ Verification successful
Windows Path Handling Verified
The
unixifyWithDriveLetterfunction is defined inpackages/utils/src/unixifyWithDriveLetter.tsand is being used in constructing glob patterns to ensure Windows paths are processed with drive letters intact. Although our search did not reveal dedicated Windows-specific tests, the function’s location and usage indicate that Windows path handling is being addressed as intended. If further assurance is necessary, consider adding explicit unit tests for Windows path scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze Windows path handling # Check unixifyWithDriveLetter implementation ast-grep --pattern 'function unixifyWithDriveLetter' # Check for test cases involving Windows paths rg -A 5 'test.*Windows'Length of output: 80
Script:
#!/bin/bash # Script to further investigate the implementation and usage of unixifyWithDriveLetter echo "=== Searching for unixifyWithDriveLetter: Function Declaration ===" rg -n 'function unixifyWithDriveLetter' echo "=== Searching for unixifyWithDriveLetter: Arrow Function Definition ===" rg -n 'const unixifyWithDriveLetter\s*=' echo "=== Searching for Windows path tests in the repository ===" rg -n "unixifyWithDriveLetter.*Windows" echo "=== Searching for general Windows path mentions for additional context ===" rg -n "Windows"Length of output: 971
|
@ardatan I think this PR is good to go |
| @@ -59,7 +58,7 @@ const FILE_EXTENSIONS = [ | |||
| '.gjs', | |||
| ]; | |||
|
|
|||
| function createGlobbyOptions(options: CodeFileLoaderOptions): GlobbyOptions { | |||
| function createGlobbyOptions(options: CodeFileLoaderOptions): GlobOptions { | |||
There was a problem hiding this comment.
Are we sure the options are the same? Otherwise it is a breaking change.
There was a problem hiding this comment.
I think some options are different, so I think I better rename it to TinyGlobbyOptions
There was a problem hiding this comment.
It looks like the options used here are cwd, absolute, and ignore. The tinyglobby package supports all three of those options
|
@talentlessguy would you be able to rebase this PR? I just loaded your branch in VS Code and confirmed it's not a breaking change |
merge master
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
packages/executor/src/execution/execute.ts (1)
744-773: Clarify AggregateError handling behavior.The error handling processes all errors in the AggregateError but only returns the result from the last error. This might lead to inconsistent behavior if different errors produce different results.
Consider returning after processing all errors or documenting why only the last result is used:
if (rawError instanceof AggregateError) { - let result: unknown; + const results: unknown[] = []; for (let rawErrorItem of rawError.errors) { rawErrorItem = coerceError(rawErrorItem); const error = locatedError(rawErrorItem, fieldNodes, pathToArray(path)); - result = handleFieldError(error, returnType, errors); + results.push(handleFieldError(error, returnType, errors)); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); } - return result; + // Return null if any error was handled (following GraphQL error propagation rules) + return results[results.length - 1]; }
♻️ Duplicate comments (2)
.github/workflows/tests.yml (1)
46-47: Same Node 24 bump in the “check” job – see previous note.packages/load/tests/loaders/documents/documents-from-glob.spec.ts (1)
23-23: Path normalization for Windows compatibilityThe
.replace(/\\/g, '/')normalization ensures Windows paths work correctly withtinyglobby. This approach was already discussed and approved in previous comments.Also applies to: 41-41
🧹 Nitpick comments (15)
packages/loaders/graphql-file/CHANGELOG.md (1)
8-13: Duplicate commit hash in bullet list – trim for brevityThe hash
a824120appears twice in the same bullet list, which is redundant noise in the changelog.- [`a824120`](https://github.com/ardatan/graphql-tools/commit/a8241205862ebcf294a835da6eb782f777e9fdfb), [`d15c850`](https://github.com/ardatan/graphql-tools/commit/d15c850d69c3b7d034828f397c8fa59ca30e9b98), [`22af985`](https://github.com/ardatan/graphql-tools/commit/22af98581e983079ec7e53677b905d3d03117524), - [`a824120`](https://github.com/ardatan/graphql-tools/commit/a8241205862ebcf294a835da6eb782f777e9fdfb), [`53db005`](https://github.com/ardatan/graphql-tools/commit/53db00540c2549748afdeeb65bb45a6c45ce57d3)]:.github/workflows/website.yml (1)
24-27: Inconsistent Node versions across workflows.The site now builds on Node 20 while tests/release run on Node 24. Divergence increases “works on CI A but not CI B” risk.
Align versions (prefer current LTS) unless there is a strong reason to pin the website to 20..github/workflows/tests.yml (2)
63-71: Matrix change drops Node 20 / 23, adds 24 – good, but watch coverage.Dropping Node 20 means we lose the oldest still-supported LTS signal. If supporting Node 20 is still in the engines field, keep one job on 20.
99-109: Retry wrapper is still limited totimeout_minutes: 10.Longer tests (especially Windows) can exceed this. Consider 15–20 min to prevent flaky “context deadline exceeded”.
.github/workflows/codeql-analysis.yml (2)
72-79: Considerbuild-mode: automaticinstead ofnone.Skipping the build can reduce precision for TS projects (no type info). Unless build is truly impossible, switch to
automatic.
92-101: The manual-build stub exits with status 1.If a future contributor sets
build-mode: manualthis unconditionalexit 1will kill the workflow. Replace with areturnor informative echo.packages/mock/tests/mocking-compatibility.spec.ts (4)
94-94: Consider using the more specifictoThrowErrormatcher.While
toThrowworks correctly here,toThrowErroris more specific when testing for exact error messages and would better document the test's intent.- expect(() => addMocksToSchema({} as any)).toThrow('Must provide schema to mock'); + expect(() => addMocksToSchema({} as any)).toThrowError('Must provide schema to mock');
98-100: Consider using the more specifictoThrowErrormatcher.For consistency and clarity when testing error messages.
- expect(() => addMocksToSchema({ schema: {} as unknown as GraphQLSchema })).toThrow( + expect(() => addMocksToSchema({ schema: {} as unknown as GraphQLSchema })).toThrowError( 'Value at "schema" must be of type GraphQLSchema', );
110-110: Consider using the more specifictoThrowErrormatcher.For consistency with error message testing.
- ).toThrow('mocks must be of type Object'); + ).toThrowError('mocks must be of type Object');
1436-1438: Consider using the more specifictoThrowErrormatcher.For consistency with other error message tests.
- expect(() => new MockList(5, 'abc' as any)).toThrow( + expect(() => new MockList(5, 'abc' as any)).toThrowError( 'Second argument to MockList must be a function or undefined', );packages/mock/src/MockStore.ts (1)
432-434: Security improvement with opportunity for refactoring.The prototype pollution protection is correctly implemented here as well. However, consider extracting the validation logic into a helper function to avoid code duplication between
setImplandinsertmethods.+private validateSafeKey(key: string, keyType: string): void { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + throw new Error(`Invalid ${keyType}: ${key}`); + } +}Then use
this.validateSafeKey(typeName, 'typeName')andthis.validateSafeKey(key, 'key')in both methods.Also applies to: 440-442
packages/merge/tests/merge-typedefs.spec.ts (1)
1796-1796: TODO: Multiple schema extensions supportThe commented test case shows the intended functionality for supporting multiple schema extensions. This would be valuable for complex federation setups.
Would you like me to help implement support for multiple schema extensions or create an issue to track this enhancement?
packages/loaders/github/src/index.ts (1)
109-109: Consider using optional chaining for cleaner codeThe static analysis tool suggests using optional chaining here.
-if (contentType && contentType.includes('application/json')) { +if (contentType?.includes('application/json')) {packages/merge/src/typedefs-mergers/directives.ts (2)
59-86: Implementation looks good.The deep value matching logic correctly handles all GraphQL value types with proper recursion.
For better performance with large lists, consider using a more efficient matching algorithm:
case Kind.LIST: + if (a.values.length !== (b as typeof a).values.length) { + return false; + } + // For large lists, consider using a hash-based approach + if (a.values.length > 100) { + const bValueSet = new Set((b as typeof a).values.map(v => JSON.stringify(v))); + return a.values.every(aVal => bValueSet.has(JSON.stringify(aVal))); + } return ( - a.values.length === (b as typeof a).values.length && a.values.every(aVal => (b as typeof a).values.find(bVal => matchValues(aVal, bVal))) );
87-102: Remove redundant boolean conversion.The double negation on line 100 is unnecessary as the expression already evaluates to a boolean.
- return !!matched; + return matched;
| npmTag: alpha | ||
| buildScript: build | ||
| nodeVersion: 22 | ||
| nodeVersion: 24 | ||
| secrets: | ||
| githubToken: ${{ secrets.GUILD_BOT_TOKEN }} | ||
| githubToken: ${{ secrets.GITHUB_TOKEN }} | ||
| npmToken: ${{ secrets.NODE_AUTH_TOKEN }} |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Node 24 may not yet be available on GitHub runners.
the-guild-org/shared-config/setup ultimately calls actions/setup-node. Unless Node 24 has been released to the runner image channel, this will fail the job.
- nodeVersion: 24
+ # stay on the latest LTS the runners guarantee
+ nodeVersion: 22(or make the version an input defaulted from the repo root).
Run a quick matrix job or check the Actions release notes before merging.
Use a supported Node version on GitHub runners
Node 24 isn’t available yet on the hosted images (and actions/setup-node will fail fetching it). Please pin to the latest LTS that runners guarantee—e.g. 22.
• File: .github/workflows/pr.yml (lines 25–30)
- nodeVersion: 24
+ # use latest LTS supported by GitHub runners
+ nodeVersion: 22📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npmTag: alpha | |
| buildScript: build | |
| nodeVersion: 22 | |
| nodeVersion: 24 | |
| secrets: | |
| githubToken: ${{ secrets.GUILD_BOT_TOKEN }} | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} | |
| npmToken: ${{ secrets.NODE_AUTH_TOKEN }} | |
| npmTag: alpha | |
| buildScript: build | |
| # use latest LTS supported by GitHub runners | |
| nodeVersion: 22 | |
| secrets: | |
| githubToken: ${{ secrets.GITHUB_TOKEN }} | |
| npmToken: ${{ secrets.NODE_AUTH_TOKEN }} |
🤖 Prompt for AI Agents
In .github/workflows/pr.yml around lines 25 to 30, the Node.js version is set to
24, which is not yet supported on GitHub hosted runners and will cause the
setup-node action to fail. Change the nodeVersion value from 24 to a supported
version like 22, which is the latest LTS guaranteed by the runners.
| nodeVersion: 24 | ||
| secrets: | ||
| githubToken: ${{ secrets.GUILD_BOT_TOKEN }} | ||
| githubToken: ${{ secrets.GITHUB_TOKEN }} | ||
| npmToken: ${{ secrets.NODE_AUTH_TOKEN }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same Node 24 concern applies here.
The stable release job must be rock-solid; consider sticking to the current LTS until Node 24 images are officially GA.
🤖 Prompt for AI Agents
In .github/workflows/release.yml around lines 20 to 23, the Node.js version is
set to 24, which is not yet officially GA. To ensure stability for the stable
release job, change the nodeVersion value to the current LTS version instead of
24. This will use a proven, stable Node.js environment for the release workflow.
| - language: javascript | ||
| build-mode: none | ||
| - language: javascript-typescript | ||
| build-mode: none | ||
| # CodeQL supports the following values keywords for 'language': 'actions', 'c-cpp', 'csharp', 'go', 'java-kotlin', 'javascript-typescript', 'python', 'ruby', 'swift' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate language entries waste CI minutes and may error out.
javascript-typescript already subsumes plain javascript.
- - language: javascript
- build-mode: none
- language: javascript-typescript
build-mode: none📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - language: javascript | |
| build-mode: none | |
| - language: javascript-typescript | |
| build-mode: none | |
| # CodeQL supports the following values keywords for 'language': 'actions', 'c-cpp', 'csharp', 'go', 'java-kotlin', 'javascript-typescript', 'python', 'ruby', 'swift' | |
| - language: javascript-typescript | |
| build-mode: none | |
| # CodeQL supports the following values keywords for 'language': 'actions', 'c-cpp', 'csharp', 'go', 'java-kotlin', 'javascript-typescript', 'python', 'ruby', 'swift' |
🤖 Prompt for AI Agents
In .github/workflows/codeql-analysis.yml around lines 50 to 54, there are
duplicate language entries for 'javascript' and 'javascript-typescript'. Since
'javascript-typescript' already includes 'javascript', remove the separate
'javascript' entry to avoid wasting CI minutes and potential errors.
| function mergeArguments(a1: readonly ArgumentNode[], a2: readonly ArgumentNode[]): ArgumentNode[] { | ||
| const result: ArgumentNode[] = [...a2]; | ||
| const result: ArgumentNode[] = []; | ||
|
|
||
| for (const argument of a1) { | ||
| for (const argument of [...a2, ...a1]) { | ||
| const existingIndex = result.findIndex(a => a.name.value === argument.name.value); | ||
|
|
||
| if (existingIndex > -1) { | ||
| if (existingIndex === -1) { | ||
| result.push(argument); | ||
| } else { | ||
| const existingArg = result[existingIndex]; | ||
|
|
||
| if (existingArg.value.kind === 'ListValue') { | ||
| const source = (existingArg.value as any).values; | ||
| const target = (argument.value as ListValueNode).values; | ||
|
|
||
| // merge values of two lists | ||
| (existingArg.value as any).values = deduplicateLists( | ||
| source, | ||
| target, | ||
| (targetVal, source) => { | ||
| (existingArg.value as ListValueNode) = { | ||
| ...existingArg.value, | ||
| values: deduplicateLists(source, target, (targetVal, source) => { | ||
| const value = (targetVal as any).value; | ||
| return !value || !source.some((sourceVal: any) => sourceVal.value === value); | ||
| }, | ||
| ); | ||
| }), | ||
| }; | ||
| } else { | ||
| (existingArg as any).value = argument.value; | ||
| } | ||
| } else { | ||
| result.push(argument); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Add type guard for list value access.
The code accesses .values property on lines 39-40 without ensuring both values are ListValueNode types. While line 38 checks existingArg.value.kind === 'ListValue', it doesn't verify that argument.value is also a list.
Apply this fix:
if (existingArg.value.kind === 'ListValue') {
+ if (argument.value.kind !== 'ListValue') {
+ result[existingIndex] = argument;
+ continue;
+ }
const source = (existingArg.value as any).values;
const target = (argument.value as ListValueNode).values;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function mergeArguments(a1: readonly ArgumentNode[], a2: readonly ArgumentNode[]): ArgumentNode[] { | |
| const result: ArgumentNode[] = [...a2]; | |
| const result: ArgumentNode[] = []; | |
| for (const argument of a1) { | |
| for (const argument of [...a2, ...a1]) { | |
| const existingIndex = result.findIndex(a => a.name.value === argument.name.value); | |
| if (existingIndex > -1) { | |
| if (existingIndex === -1) { | |
| result.push(argument); | |
| } else { | |
| const existingArg = result[existingIndex]; | |
| if (existingArg.value.kind === 'ListValue') { | |
| const source = (existingArg.value as any).values; | |
| const target = (argument.value as ListValueNode).values; | |
| // merge values of two lists | |
| (existingArg.value as any).values = deduplicateLists( | |
| source, | |
| target, | |
| (targetVal, source) => { | |
| (existingArg.value as ListValueNode) = { | |
| ...existingArg.value, | |
| values: deduplicateLists(source, target, (targetVal, source) => { | |
| const value = (targetVal as any).value; | |
| return !value || !source.some((sourceVal: any) => sourceVal.value === value); | |
| }, | |
| ); | |
| }), | |
| }; | |
| } else { | |
| (existingArg as any).value = argument.value; | |
| } | |
| } else { | |
| result.push(argument); | |
| } | |
| } | |
| return result; | |
| } | |
| function mergeArguments(a1: readonly ArgumentNode[], a2: readonly ArgumentNode[]): ArgumentNode[] { | |
| const result: ArgumentNode[] = []; | |
| for (const argument of [...a2, ...a1]) { | |
| const existingIndex = result.findIndex(a => a.name.value === argument.name.value); | |
| if (existingIndex === -1) { | |
| result.push(argument); | |
| } else { | |
| const existingArg = result[existingIndex]; | |
| if (existingArg.value.kind === 'ListValue') { | |
| if (argument.value.kind !== 'ListValue') { | |
| result[existingIndex] = argument; | |
| continue; | |
| } | |
| const source = (existingArg.value as any).values; | |
| const target = (argument.value as ListValueNode).values; | |
| // merge values of two lists | |
| (existingArg.value as ListValueNode) = { | |
| ...existingArg.value, | |
| values: deduplicateLists(source, target, (targetVal, source) => { | |
| const value = (targetVal as any).value; | |
| return !value || !source.some((sourceVal: any) => sourceVal.value === value); | |
| }), | |
| }; | |
| } else { | |
| (existingArg as any).value = argument.value; | |
| } | |
| } | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In packages/merge/src/typedefs-mergers/directives.ts lines 27 to 57, the code
assumes argument.value is a ListValueNode without checking its kind, which can
cause runtime errors. Add a type guard to verify that argument.value.kind is
'ListValue' before accessing its values property. Only perform the list merging
logic if both existingArg.value and argument.value are confirmed to be
ListValueNode types; otherwise, handle the value assignment as before.
| function importFederatedSchemaLinks( | ||
| definition: DefinitionNode, | ||
| definitionsByName: Map<string, Set<DefinitionNode>>, | ||
| ) { | ||
| const addDefinition = (name: string) => { | ||
| definitionsByName.set(name.replace(/^@/g, ''), new Set()); | ||
| }; | ||
|
|
||
| // extract links from this definition | ||
| const { links, matchesImplementation, resolveImportName } = extractLinkImplementations({ | ||
| kind: Kind.DOCUMENT, | ||
| definitions: [definition], | ||
| }); | ||
|
|
||
| if (links.length) { | ||
| const federationUrl = 'https://specs.apollo.dev/federation'; | ||
| const linkUrl = 'https://specs.apollo.dev/link'; | ||
|
|
||
| /** | ||
| * Official Federated imports are special because they can be referenced without specifyin the import. | ||
| * To handle this case, we must prepare a list of all the possible valid usages to check against. | ||
| * Note that this versioning is not technically correct, since some definitions are after v2.0. | ||
| * But this is enough information to be comfortable not blocking the imports at this phase. It's | ||
| * the job of the composer to validate the versions. | ||
| * */ | ||
| if (matchesImplementation(federationUrl, 'v2.0')) { | ||
| const federationImports = [ | ||
| '@composeDirective', | ||
| '@extends', | ||
| '@external', | ||
| '@inaccessible', | ||
| '@interfaceObject', | ||
| '@key', | ||
| '@override', | ||
| '@provides', | ||
| '@requires', | ||
| '@shareable', | ||
| '@tag', | ||
| 'FieldSet', | ||
| ]; | ||
| for (const i of federationImports) { | ||
| addDefinition(resolveImportName(federationUrl, i)); | ||
| } | ||
| } | ||
| if (matchesImplementation(linkUrl, 'v1.0')) { | ||
| const linkImports = ['Purpose', 'Import', '@link']; | ||
| for (const i of linkImports) { | ||
| addDefinition(resolveImportName(linkUrl, i)); | ||
| } | ||
| } | ||
|
|
||
| const imported = links | ||
| .filter(l => ![linkUrl, federationUrl].includes(l.identity)) | ||
| .flatMap(l => l.imports.map(i => i.as ?? i.name)); | ||
| for (const namedImport of imported) { | ||
| addDefinition(namedImport); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Federation link import handling looks good.
The implementation correctly extracts and processes federation schema links with appropriate handling for official imports.
The hardcoded versions ('v2.0' for federation, 'v1.0' for link) should be verified against the latest federation specifications:
🌐 Web query:
What are the latest versions of Apollo Federation and the @link directive specification?
💡 Result:
The latest version of Apollo Federation as of July 2025 is v2.11, which was first released in June 2025. Federation v2.11 is now the recommended version and is also a prerequisite for the latest "Connector specification" features[1].
The @link directive specification is found at link v1.0. This specification provides the core definitions for the @link and @id directives, which are fundamental for linking external specifications in federated schemas[9]. The @link directive itself is required for all Apollo Federation 2 subgraph schemas to import federation-specific directives, and its specification has stabilized at v1.0[2][9].
Summary Table:
| Specification | Latest Version | Release Date |
|---|---|---|
| Apollo Federation | v2.11 | June 2025 |
| @link directive ("link") | v1.0 | Currently stable |
You can apply the latest Federation features to your schema using @link in this way:
extend schema
@link(url: "https://specs.apollo.dev/federation/v2.11", import: ["@key", "@shareable", ...])and link to the @link specification itself using:
@link(url: "https://specs.apollo.dev/link/v1.0", import: ["@link"])[1][2][3][9].
Citations:
- 1: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/versions
- 2: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/directives
- 3: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/subgraph-spec
- 4: https://www.apollographql.com/blog/whats-new-in-graphos-apollo-winter-25-release-apollo-connectors-native-query-planner-improved-tools-and-more
- 5: https://expediagroup.github.io/graphql-kotlin/docs/schema-generator/federation/federated-directives/
- 6: https://github.com/apollographql/federation/blob/main/composition-js/CHANGELOG.md
- 7: https://github.com/apollographql/federation/blob/main/ROADMAP.md
- 8: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/link
- 9: https://specs.apollo.dev
- 10: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/moving-to-federation-2
Update hardcoded federation version to v2.11
The code currently checks for federation spec version 'v2.0', but the latest Apollo Federation release is v2.11 (June 2025). Please bump this version constant to stay in sync with the official spec.
• File: packages/import/src/index.ts
Replace:
- if (matchesImplementation(federationUrl, 'v2.0')) {
+ if (matchesImplementation(federationUrl, 'v2.11')) {• Update the comment above to reflect version v2.11 instead of “v2.0”.
This ensures your linker will recognize all directives from the current federated spec.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function importFederatedSchemaLinks( | |
| definition: DefinitionNode, | |
| definitionsByName: Map<string, Set<DefinitionNode>>, | |
| ) { | |
| const addDefinition = (name: string) => { | |
| definitionsByName.set(name.replace(/^@/g, ''), new Set()); | |
| }; | |
| // extract links from this definition | |
| const { links, matchesImplementation, resolveImportName } = extractLinkImplementations({ | |
| kind: Kind.DOCUMENT, | |
| definitions: [definition], | |
| }); | |
| if (links.length) { | |
| const federationUrl = 'https://specs.apollo.dev/federation'; | |
| const linkUrl = 'https://specs.apollo.dev/link'; | |
| /** | |
| * Official Federated imports are special because they can be referenced without specifyin the import. | |
| * To handle this case, we must prepare a list of all the possible valid usages to check against. | |
| * Note that this versioning is not technically correct, since some definitions are after v2.0. | |
| * But this is enough information to be comfortable not blocking the imports at this phase. It's | |
| * the job of the composer to validate the versions. | |
| * */ | |
| if (matchesImplementation(federationUrl, 'v2.0')) { | |
| const federationImports = [ | |
| '@composeDirective', | |
| '@extends', | |
| '@external', | |
| '@inaccessible', | |
| '@interfaceObject', | |
| '@key', | |
| '@override', | |
| '@provides', | |
| '@requires', | |
| '@shareable', | |
| '@tag', | |
| 'FieldSet', | |
| ]; | |
| for (const i of federationImports) { | |
| addDefinition(resolveImportName(federationUrl, i)); | |
| } | |
| } | |
| if (matchesImplementation(linkUrl, 'v1.0')) { | |
| const linkImports = ['Purpose', 'Import', '@link']; | |
| for (const i of linkImports) { | |
| addDefinition(resolveImportName(linkUrl, i)); | |
| } | |
| } | |
| const imported = links | |
| .filter(l => ![linkUrl, federationUrl].includes(l.identity)) | |
| .flatMap(l => l.imports.map(i => i.as ?? i.name)); | |
| for (const namedImport of imported) { | |
| addDefinition(namedImport); | |
| } | |
| } | |
| } | |
| function importFederatedSchemaLinks( | |
| definition: DefinitionNode, | |
| definitionsByName: Map<string, Set<DefinitionNode>>, | |
| ) { | |
| const addDefinition = (name: string) => { | |
| definitionsByName.set(name.replace(/^@/g, ''), new Set()); | |
| }; | |
| // extract links from this definition | |
| const { links, matchesImplementation, resolveImportName } = extractLinkImplementations({ | |
| kind: Kind.DOCUMENT, | |
| definitions: [definition], | |
| }); | |
| if (links.length) { | |
| const federationUrl = 'https://specs.apollo.dev/federation'; | |
| const linkUrl = 'https://specs.apollo.dev/link'; | |
| /** | |
| * Official Federated imports are special because they can be referenced without specifying the import. | |
| * To handle this case, we must prepare a list of all the possible valid usages to check against. | |
| * Note that this versioning is not technically correct, since some definitions are after v2.0. | |
| * But this is enough information to be comfortable not blocking the imports at this phase. It's | |
| * the job of the composer to validate the versions. | |
| */ | |
| - if (matchesImplementation(federationUrl, 'v2.0')) { | |
| + if (matchesImplementation(federationUrl, 'v2.11')) { | |
| const federationImports = [ | |
| '@composeDirective', | |
| '@extends', | |
| '@external', | |
| '@inaccessible', | |
| '@interfaceObject', | |
| '@key', | |
| '@override', | |
| '@provides', | |
| '@requires', | |
| '@shareable', | |
| '@tag', | |
| 'FieldSet', | |
| ]; | |
| for (const i of federationImports) { | |
| addDefinition(resolveImportName(federationUrl, i)); | |
| } | |
| } | |
| if (matchesImplementation(linkUrl, 'v1.0')) { | |
| const linkImports = ['Purpose', 'Import', '@link']; | |
| for (const i of linkImports) { | |
| addDefinition(resolveImportName(linkUrl, i)); | |
| } | |
| } | |
| const imported = links | |
| .filter(l => ![linkUrl, federationUrl].includes(l.identity)) | |
| .flatMap(l => l.imports.map(i => i.as ?? i.name)); | |
| for (const namedImport of imported) { | |
| addDefinition(namedImport); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/import/src/index.ts between lines 266 and 325, update the hardcoded
federation version string from 'v2.0' to 'v2.11' in the matchesImplementation
call to reflect the latest Apollo Federation release. Also, revise the comment
above this code block to mention version v2.11 instead of v2.0, ensuring the
documentation matches the updated version check.
| if (options?.subscriptionsProtocol === SubscriptionProtocol.SSE) { | ||
| return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as any, options); | ||
| return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as AsyncFetchFn, options); | ||
| } else if (options?.subscriptionsProtocol === SubscriptionProtocol.GRAPHQL_SSE) { | ||
| if (!options?.subscriptionsEndpoint) { | ||
| // when no custom subscriptions endpoint is specified, | ||
| // graphql-sse is recommended to be used on `/graphql/stream` | ||
| subscriptionsEndpoint += '/stream'; | ||
| } | ||
| return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as any, options); | ||
| return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as AsyncFetchFn, options); | ||
| } else { | ||
| const webSocketImpl$ = new ValueOrPromise(() => this.getWebSocketImpl(importFn, options)); | ||
| const executor$ = webSocketImpl$.then(webSocketImpl => { | ||
| if (options?.subscriptionsProtocol === SubscriptionProtocol.LEGACY_WS) { | ||
| return this.buildWSLegacyExecutor(subscriptionsEndpoint, webSocketImpl, options); | ||
| } else { | ||
| return this.buildWSExecutor( | ||
| subscriptionsEndpoint, | ||
| webSocketImpl, | ||
| options?.connectionParams, | ||
| ); | ||
| } | ||
| }); | ||
| return request => executor$.then(executor => executor(request)).resolve(); | ||
| return request => | ||
| handleMaybePromise( | ||
| () => | ||
| handleMaybePromise( | ||
| () => this.getWebSocketImpl(importFn, options), | ||
| webSocketImpl => { | ||
| if (options?.subscriptionsProtocol === SubscriptionProtocol.LEGACY_WS) { | ||
| return this.buildWSLegacyExecutor(subscriptionsEndpoint, webSocketImpl, options); | ||
| } else { | ||
| return this.buildWSExecutor( | ||
| subscriptionsEndpoint, | ||
| webSocketImpl, | ||
| options?.connectionParams, | ||
| ); | ||
| } | ||
| }, | ||
| ), | ||
| executor => executor(request), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential type safety issue with fetch casting.
The code casts fetch to AsyncFetchFn for SSE protocols without verifying the actual type. This could cause runtime errors if a synchronous fetch is provided.
Consider adding type guards or separate handling:
if (options?.subscriptionsProtocol === SubscriptionProtocol.SSE) {
- return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as AsyncFetchFn, options);
+ if (importFn === asyncImport) {
+ return this.buildHTTPExecutor(subscriptionsEndpoint, fetch as AsyncFetchFn, options);
+ } else {
+ throw new Error('SSE subscriptions require async fetch implementation');
+ }
} else if (options?.subscriptionsProtocol === SubscriptionProtocol.GRAPHQL_SSE) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/loaders/url/src/index.ts around lines 269 to 298, the code casts
fetch to AsyncFetchFn without verifying its type, risking runtime errors if
fetch is synchronous. To fix this, add a type guard to check if fetch matches
AsyncFetchFn before casting, or provide separate handling for synchronous fetch
functions to ensure type safety and prevent runtime issues.
🚨 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
Cleans up some dependencies without introducing breaking changes that much.
Related #6895
Type of change
Please delete options that are not relevant.
expected)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
bun testTest Environment:
@graphql-tools/...:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...