Closed
Conversation
…into ondrej/eng-3812-implement-composedirective-directive
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors directive representation from "persisted directives" to "federated directives" across the composition system, introduces new directive-definition-data infrastructure with typed parameters and utilities, creates a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2806 +/- ##
==========================================
- Coverage 46.28% 40.28% -6.00%
==========================================
Files 1045 762 -283
Lines 139782 100686 -39096
Branches 8768 5142 -3626
==========================================
- Hits 64702 40565 -24137
+ Misses 73328 58421 -14907
+ Partials 1752 1700 -52
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
composition/src/normalization/normalization.ts (1)
1-34:⚠️ Potential issue | 🟠 MajorKeep a compatibility path for
batchNormalize.Dropping
batchNormalizefrom this entrypoint also removes it from the package root, becausecomposition/src/index.tsre-exports./normalization/normalization. Any existingimport { batchNormalize } ...consumer now breaks unless this is staged as an explicit breaking change or kept behind a deprecated shim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/normalization/normalization.ts` around lines 1 - 34, The package removed the batchNormalize export from this normalization entrypoint causing downstream imports (import { batchNormalize } ...) to break; restore a compatibility export by reintroducing batchNormalize (or a deprecated shim that calls the new implementation) alongside normalizeSubgraph and normalizeSubgraphFromString so composition's re-export still exposes batchNormalize; locate where normalizeSubgraph/normalizeSubgraphFromString are forwarded (functions normalizeSubgraph, normalizeSubgraphFromString and the V1 implementations normalizeSubgraphV1/normalizeSubgraphFromStringV1) and add an exported function named batchNormalize that delegates to the appropriate V1 batchNormalize implementation (or wraps it and emits a deprecation warning) to preserve backwards compatibility.composition/src/v1/normalization/utils.ts (1)
467-512:⚠️ Potential issue | 🔴 CriticalDon't reuse mutable
DirectiveDefinitionDatasingletons here.
initializeDirectiveDefinitionDatas()andFEDERATED_DIRECTIVE_DATASboth hand out the imported*_DEFINITION_DATAobjects by reference. Those instances are later mutated incomposition/src/v1/normalization/normalization-factory.tsLines 767-768 and in this file on Lines 531-536, so one normalization run can leakversion,isComposed, ordescriptioninto the next. Please clone or rehydrate freshDirectiveDefinitionDatainstances before storing them in these collections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/utils.ts` around lines 467 - 512, The collection initializers are storing references to mutable singleton DirectiveDefinitionData objects, causing mutations (e.g., version, isComposed, description) in normalization runs to leak across runs; update initializeDirectiveDefinitionDatas() and FEDERATED_DIRECTIVE_DATAS to store fresh copies instead of the imported *_DEFINITION_DATA singletons — for each symbol like AUTHENTICATED_DEFINITION_DATA, COMPOSE_DIRECTIVE_DEFINITION_DATA, etc., create a new DirectiveDefinitionData instance or shallow-clone the object (so fields such as version/isComposed/description are not shared) before inserting into the Map or the FEDERATED_DIRECTIVE_DATAS array; ensure the same cloning approach is used wherever those collections are built so normalization-factory.ts mutations no longer affect subsequent runs.
🧹 Nitpick comments (12)
composition/src/utils/params.ts (1)
16-19: Use aninterfacefor this object-shape params contract.
MergeSetValueMapParamsis modeling an object shape and should be aninterfaceper repo TypeScript guidelines.♻️ Proposed change
-export type MergeSetValueMapParams<K, V> = { +export interface MergeSetValueMapParams<K, V> { source: Map<K, Set<V>>; target: Map<K, Set<V>>; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/utils/params.ts` around lines 16 - 19, Replace the type alias with an interface for the object-shaped params: change the declaration of MergeSetValueMapParams to an interface that declares source: Map<K, Set<V>> and target: Map<K, Set<V>> so it follows the repo TypeScript guideline to use interfaces for object-shaped contracts; keep the generic parameters <K, V> and the property names exactly as in MergeSetValueMapParams to preserve usage compatibility.composition/src/errors/types/types.ts (1)
3-6: Use an interface instead of a type alias for this object shape.Line 3 defines an object shape with
type; switch tointerfaceto match repo standards.♻️ Proposed change
-export type InvalidRootTypeFieldEventsDirectiveData = { +export interface InvalidRootTypeFieldEventsDirectiveData { definesDirectives: boolean; invalidDirectiveNames: Array<DirectiveName>; -}; +}As per coding guidelines,
**/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/errors/types/types.ts` around lines 3 - 6, Replace the exported type alias InvalidRootTypeFieldEventsDirectiveData with an exported interface of the same name (preserving the property names and types: definesDirectives: boolean and invalidDirectiveNames: Array<DirectiveName>) so the object shape uses an interface instead of a type; keep the export and the DirectiveName reference unchanged.composition/src/subgraph/types/params.ts (1)
4-7: Prefer an interface for this params object shape.Line 4 currently uses a type alias for an object shape; this repo standard prefers an interface.
♻️ Proposed change
-export type InternalSubgraphFromNormalizationParams = { +export interface InternalSubgraphFromNormalizationParams { normalization: NormalizationSuccess; subgraphName: SubgraphName; -}; +}As per coding guidelines,
**/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/subgraph/types/params.ts` around lines 4 - 7, Replace the exported type alias InternalSubgraphFromNormalizationParams with an exported interface of the same name describing the same properties (normalization: NormalizationSuccess; subgraphName: SubgraphName) so the object shape uses an interface instead of a type alias; keep the same exported identifier and referenced types (NormalizationSuccess, SubgraphName) so all callers remain compatible.composition/src/schema-building/types/params.ts (1)
13-75: Prefer interfaces for object-shape params in this new TS types module.These declarations are all object-shape aliases; convert to
interfacefor consistency with repo TS guidelines.♻️ Refactor pattern
-export type IsTypeValidImplementationParams = { +export interface IsTypeValidImplementationParams { concreteTypeNamesByAbstractTypeName: Map<TypeName, Set<TypeName>>; implementationType: TypeNode; interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName, Set<InterfaceTypeName>>; originalType: TypeNode; -}; +}As per coding guidelines
**/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/types/params.ts` around lines 13 - 75, Several exported object-shaped type aliases (e.g., IsTypeValidImplementationParams, GetRouterPersistedDirectiveNodesParams, GetValidArgumentNodesParams, GetValidExecutableDirectiveArgumentNodesParams, DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams, RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams, RouterSchemaNodeFromDataParams, CompareAndValidateInputDefaultValuesParams) should be converted to interfaces; change each "export type X = { ... }" to "export interface X { ... }", preserving all property names and types (including Maps, unions, optional fields like node? and description?), keep the same exported identifiers so external usage/imports remain valid, and run TypeScript type-check to confirm no downstream changes are required.composition/src/validation/types/params.ts (1)
6-26: Use interfaces for these exported parameter objects.All three additions are parameter-bag object shapes, so exporting them as
interfaces would align this module with the repo’s TypeScript conventions.As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/validation/types/params.ts` around lines 6 - 26, Replace the three exported object-shape type aliases IsArgumentValueValidParams, ValidateCustomDirectiveParams, and ValidateDirectivesParams with exported interfaces of the same names and identical member signatures (argumentValue, parentDefinitionDataByTypeName, typeNode for IsArgumentValueValidParams; argumentDataByName, directiveNode, parentDefinitionDataByTypeName, requiredArgumentNames for ValidateCustomDirectiveParams; data, directiveCoords, directiveDefinitionData, directiveNodes, parentDefinitionDataByTypeName for ValidateDirectivesParams) so the module follows the repo convention of using interfaces for parameter-bag objects; keep all property types and export names unchanged.composition/src/schema-building/types/types.ts (1)
181-186: Prefer aninterfaceforFederatedDirectivesData.This is a plain exported object contract, so making it an
interfacewould keep it aligned with the repo’s TypeScript style.As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/types/types.ts` around lines 181 - 186, Replace the exported type alias FederatedDirectivesData with an exported interface of the same name and identical members (deprecatedReason: string; directivesByName: Map<DirectiveName, ConstDirectiveNode[]>; isDeprecated: boolean; tagDirectiveByName: Map<string, ConstDirectiveNode>), so callers continue to work without behavior changes; update any nearby JSDoc/comments if needed and ensure the exported symbol name remains FederatedDirectivesData.composition/src/v1/normalization/batch-normalization/types/params.ts (1)
3-7: Prefer aninterfacefor this exported params bag.
HandleOverridesParamsis a plain object contract, so using aninterfacehere would match the repo’s TS style and keep future extension simpler.As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/batch-normalization/types/params.ts` around lines 3 - 7, Replace the exported type alias HandleOverridesParams with an exported interface of the same name and identical properties (originalTypeNameByRenamedTypeName, overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the object contract is defined as an interface (preserve the Map/Set types and their names) to match the repo style and enable easier future extension; update any local references if needed but keep the exported symbol name unchanged.composition/src/v1/federation/federation-factory.ts (1)
1507-1549: Drop the dead persisted-directive stub.Keeping the old implementation fully commented-out here makes the new directive merge path harder to follow and leaves removed terminology in the hot path. A short TODO or issue link is easier to maintain than carrying the retired code inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` around lines 1507 - 1549, Remove the large commented-out implementation inside upsertPersistedDirectiveDefinitionData(): delete the dead code block (the commented logic referencing potentialPersistedDirectiveDefinitionDataByName, setMutualExecutableLocations, upsertInputValueData, setLongestDescription, addIterableToSet, etc.) and either remove the empty function if it's unused or replace it with a single-line TODO/issue-link comment explaining why persisted-directive logic was retired; ensure no removed symbols are referenced elsewhere or update callsites accordingly.composition/src/index.ts (1)
3-5: Remove the duplicate barrel re-exports.
./directive-definition-data/directive-definition-dataand./v1/constants/constantsare both exported twice. It is harmless, but it makes the public surface harder to audit and easier to create merge conflicts around.Also applies to: 41-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/index.ts` around lines 3 - 5, The file contains duplicate barrel re-exports (e.g., the export of './directive-definition-data/directive-definition-data' and './v1/constants/constants' appear more than once); remove the duplicate export statements so each module is exported only once—locate the repeated export lines for directive-definition-data and v1/constants in index.ts (also the repeated block around lines 41-52) and delete the redundant entries, leaving a single canonical export for each module.composition/src/directive-definition-data/types/params.ts (1)
21-67: Use interfaces for these exported param objects.These new declarations are all object shapes, and the repo guideline prefers
interfacehere overtype = { ... }.As per coding guidelines "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/directive-definition-data/types/params.ts` around lines 21 - 67, Change the exported object-shaped type aliases to exported interfaces: replace the type declarations for UpsertFederatedDirectiveDataParams, AddDirectiveArgumentDataByNodeParams, DirectiveArgumentDataParams, DirectiveDefinitionDataParams, and ExtractDirectiveArgumentDataParams with equivalent exported interfaces, preserving all property names, optional modifiers (e.g. ?), exact property types (Maps, Sets, unions like InputNodeKind | Kind.NULL), comments (e.g. the commented configureDescription line), and exported status so consumers and imports remain unchanged.composition/src/schema-building/utils.ts (1)
211-216: Use strict equality for the printed default-value comparison.This is a string-to-string comparison, so
===keeps it consistent with the repo rule and avoids introducing a loose-equality exception into the new path.As per coding guidelines "Always use strict equality (
===and!==) instead of loose equality (==and!=)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/utils.ts` around lines 211 - 216, Replace the loose equality check between printed default-value strings with a strict equality check: in the block that computes existingDefaultValueString = print(existingData.defaultValue) and incomingDefaultValueString = print(incomingData.defaultValue), change the comparison from `==` to `===` so the early-return path in the function uses strict equality (`existingDefaultValueString === incomingDefaultValueString`) consistent with project guidelines.composition/src/normalization/types.ts (1)
36-63: Prefer interfaces for these result payloads.
NormalizationSuccessandBatchNormalizationSuccessare plain object contracts, so keeping them as interfaces would align better with the repo TS convention and make later extension/composition easier.As per coding guidelines "Prefer interfaces over type aliases for object shapes in TypeScript".
Also applies to: 72-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/normalization/types.ts` around lines 36 - 63, Replace the object type aliases with interfaces for the normalization result payloads: change the NormalizationSuccess type declaration to an interface named NormalizationSuccess and do the same for BatchNormalizationSuccess (the other alias referenced in the comment around lines 72-84), keeping all property names and types identical; ensure exported declarations remain exported and update any import/usage sites if they rely on type-only constructs so the interface names are used consistently across functions like any normalizer return types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/node/action.yaml:
- Around line 5-7: The description for the node-version input is inconsistent
with the actual default; update the "description" string for the node-version
input so it matches the configured default value ('22.x') or change the
"default" to 'lts' if you intend to keep the original message—modify the
description line that currently reads "The node version to install (Default:
lts)" to reflect "Default: 22.x" (or set default to 'lts') so the node-version
input's description and the default: '22.x' stay aligned.
In `@composition/package.json`:
- Line 43: Update the composition package catalog entry for "@types/node" so it
permits Node 24 typings as well as 22; specifically replace the current catalog
pin ("@types/node": "catalog:") value with a version range that covers both
major versions (for example "^22.0.0 || ^24.0.0" or another appropriate
inclusive range) so TypeScript will resolve `@types/node` matching Node 24 when
running on Node 24, and ensure the change is made to the "@types/node"
dependency entry in composition/package.json.
In `@composition/src/schema-building/utils.ts`:
- Around line 600-602: When building schema nodes, ensure you clear
data.node.defaultValue when data.includeDefaultValue is false so previous
defaults don't leak; in the code handling data.includeDefaultValue (refer to the
data.includeDefaultValue check and the mutated data.node.defaultValue
assignment), add logic to remove or set data.node.defaultValue to undefined/null
in the else path (or before returning) so that when includeDefaultValue is false
any previously-set default is explicitly cleared.
- Around line 472-484: The current early-return path skips validation for single
directive instances; remove or change the short-circuit so that you always run
validateDirectives on the (possibly single) directive node(s). Specifically,
stop pushing directiveNodes directly and instead call
extractUniqueDirectiveNodes(directiveNodes) and then validateDirectives({ data,
directiveCoords: coords, directiveDefinitionData: directiveData, directiveNodes:
uniqueDirectiveNodes, parentDefinitionDataByTypeName }) for all cases (including
when length === 1); only push nodes into nodes after validation passes.
In `@composition/src/subgraph/utils.ts`:
- Line 21: The constructor/creator for InternalSubgraph is currently resetting
overriddenFieldNamesByParentTypeName to a new empty Map, which drops normalized
override metadata; instead copy or clone the existing override map from the
source subgraph into the new InternalSubgraph so override-aware logic keeps
working. Locate where InternalSubgraph is built (the place that sets
overriddenFieldNamesByParentTypeName) and replace the new Map() assignment with
a shallow copy/clone of the original map (preserving each Set<FieldName> per
TypeName) so callers that rely on overriddenFieldNamesByParentTypeName continue
to see the normalized override data.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 733-745: handleComposeDirective currently assumes the first
argument is a string and calls startsWith on rawValue; add a guard in
handleComposeDirective (operating on the ComposeDirectiveNode and its argNode)
to verify the first argument is a string literal before using string
methods—e.g., check argNode.value.kind === 'StringValue' or typeof
argNode.value.value === 'string' and if that check fails push a composition
error (or reuse the existing directive error collection) and return early so
malformed non-string values (objects/booleans) do not cause a runtime exception
when evaluating rawValue.startsWith(LITERAL_AT).
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 9-31: The ComposeDirectiveNode and ComposeDirectiveArgumentNode
types reference loc?: Location but Location isn't imported from 'graphql';
update the existing graphql import (the block that currently imports
ConstDirectiveNode, DocumentNode, InputValueDefinitionNode, Kind, NameNode,
StringValueNode, ValueNode) to also import the type Location so the loc property
resolves correctly for those types.
In `@composition/src/v1/warnings/warnings.ts`:
- Around line 211-238: The two warning message strings need copy fixes: in
composedOneOfDirectiveWarning update the concatenated message so "federated.
schema" becomes "federated schema" (remove the stray period and join sentences
cleanly) and in invalidRepeatedComposedDirectiveWarning remove the double space
in "defined as" (make it "defined as") and tidy any surrounding
spacing/concatenation so the final Warning message reads smoothly; locate these
messages in the functions composedOneOfDirectiveWarning and
invalidRepeatedComposedDirectiveWarning and adjust the template strings
accordingly.
In `@composition/src/validation/validation.ts`:
- Around line 118-122: The current early return only checks that the named type
is an INPUT_OBJECT_TYPE_DEFINITION and that the argumentValue is an OBJECT, but
does not validate fields; replace the TODO by performing a deep comparison: when
parentData.kind === Kind.INPUT_OBJECT_TYPE_DEFINITION and argumentValue.kind ===
Kind.OBJECT, iterate the input object type definition (parentData) fields and
verify required fields are present, each provided field matches the field's
input type (recursively validate nested OBJECT/LIST/SCALAR values), and reject
unexpected fields not defined on parentData; reuse or call the existing
value-validation helper (or implement a recursive validateValueAgainstInputType
function) to validate nested values and ensure non-nullable constraints and
default behavior are enforced for symbols parentData, argumentValue, and Kind.
- Around line 108-110: The validator currently allows quoted strings for
enum-typed directive arguments by checking argumentValue.kind against
Kind.STRING; update the check in the block where parentData.kind ===
Kind.ENUM_TYPE_DEFINITION to reject Kind.STRING and only accept Kind.ENUM (i.e.,
remove or change the branch that treats Kind.STRING as valid) so
argumentValue.kind must equal Kind.ENUM to return true and everything else
returns false; adjust the logic around parentData.kind,
Kind.ENUM_TYPE_DEFINITION, and argumentValue.kind accordingly.
In `@composition/tests/v1/directives/compose-directive.test.ts`:
- Line 250: The placeholder test "that different core features can import the
same named directive if only one is composed" is empty and gives false coverage;
either implement real assertions that exercise the scenario or remove the test
until covered. Implementing: create two mock core feature modules that both
import a directive with the same name, compose only one of them via the
composeDirective/composeFeature helper used elsewhere in this suite, then assert
that importing the uncomposed feature does not produce a composed directive (or
produces a fallback) while the composed feature yields the composed directive;
use the existing test helpers and functions in compose-directive.test.ts (the
test block with that description) to locate where to add these assertions.
Ensure assertions check both feature-level import behavior and that only one
composed directive instance exists.
---
Outside diff comments:
In `@composition/src/normalization/normalization.ts`:
- Around line 1-34: The package removed the batchNormalize export from this
normalization entrypoint causing downstream imports (import { batchNormalize }
...) to break; restore a compatibility export by reintroducing batchNormalize
(or a deprecated shim that calls the new implementation) alongside
normalizeSubgraph and normalizeSubgraphFromString so composition's re-export
still exposes batchNormalize; locate where
normalizeSubgraph/normalizeSubgraphFromString are forwarded (functions
normalizeSubgraph, normalizeSubgraphFromString and the V1 implementations
normalizeSubgraphV1/normalizeSubgraphFromStringV1) and add an exported function
named batchNormalize that delegates to the appropriate V1 batchNormalize
implementation (or wraps it and emits a deprecation warning) to preserve
backwards compatibility.
In `@composition/src/v1/normalization/utils.ts`:
- Around line 467-512: The collection initializers are storing references to
mutable singleton DirectiveDefinitionData objects, causing mutations (e.g.,
version, isComposed, description) in normalization runs to leak across runs;
update initializeDirectiveDefinitionDatas() and FEDERATED_DIRECTIVE_DATAS to
store fresh copies instead of the imported *_DEFINITION_DATA singletons — for
each symbol like AUTHENTICATED_DEFINITION_DATA,
COMPOSE_DIRECTIVE_DEFINITION_DATA, etc., create a new DirectiveDefinitionData
instance or shallow-clone the object (so fields such as
version/isComposed/description are not shared) before inserting into the Map or
the FEDERATED_DIRECTIVE_DATAS array; ensure the same cloning approach is used
wherever those collections are built so normalization-factory.ts mutations no
longer affect subsequent runs.
---
Nitpick comments:
In `@composition/src/directive-definition-data/types/params.ts`:
- Around line 21-67: Change the exported object-shaped type aliases to exported
interfaces: replace the type declarations for
UpsertFederatedDirectiveDataParams, AddDirectiveArgumentDataByNodeParams,
DirectiveArgumentDataParams, DirectiveDefinitionDataParams, and
ExtractDirectiveArgumentDataParams with equivalent exported interfaces,
preserving all property names, optional modifiers (e.g. ?), exact property types
(Maps, Sets, unions like InputNodeKind | Kind.NULL), comments (e.g. the
commented configureDescription line), and exported status so consumers and
imports remain unchanged.
In `@composition/src/errors/types/types.ts`:
- Around line 3-6: Replace the exported type alias
InvalidRootTypeFieldEventsDirectiveData with an exported interface of the same
name (preserving the property names and types: definesDirectives: boolean and
invalidDirectiveNames: Array<DirectiveName>) so the object shape uses an
interface instead of a type; keep the export and the DirectiveName reference
unchanged.
In `@composition/src/index.ts`:
- Around line 3-5: The file contains duplicate barrel re-exports (e.g., the
export of './directive-definition-data/directive-definition-data' and
'./v1/constants/constants' appear more than once); remove the duplicate export
statements so each module is exported only once—locate the repeated export lines
for directive-definition-data and v1/constants in index.ts (also the repeated
block around lines 41-52) and delete the redundant entries, leaving a single
canonical export for each module.
In `@composition/src/normalization/types.ts`:
- Around line 36-63: Replace the object type aliases with interfaces for the
normalization result payloads: change the NormalizationSuccess type declaration
to an interface named NormalizationSuccess and do the same for
BatchNormalizationSuccess (the other alias referenced in the comment around
lines 72-84), keeping all property names and types identical; ensure exported
declarations remain exported and update any import/usage sites if they rely on
type-only constructs so the interface names are used consistently across
functions like any normalizer return types.
In `@composition/src/schema-building/types/params.ts`:
- Around line 13-75: Several exported object-shaped type aliases (e.g.,
IsTypeValidImplementationParams, GetRouterPersistedDirectiveNodesParams,
GetValidArgumentNodesParams, GetValidExecutableDirectiveArgumentNodesParams,
DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams,
RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams,
RouterSchemaNodeFromDataParams, CompareAndValidateInputDefaultValuesParams)
should be converted to interfaces; change each "export type X = { ... }" to
"export interface X { ... }", preserving all property names and types (including
Maps, unions, optional fields like node? and description?), keep the same
exported identifiers so external usage/imports remain valid, and run TypeScript
type-check to confirm no downstream changes are required.
In `@composition/src/schema-building/types/types.ts`:
- Around line 181-186: Replace the exported type alias FederatedDirectivesData
with an exported interface of the same name and identical members
(deprecatedReason: string; directivesByName: Map<DirectiveName,
ConstDirectiveNode[]>; isDeprecated: boolean; tagDirectiveByName: Map<string,
ConstDirectiveNode>), so callers continue to work without behavior changes;
update any nearby JSDoc/comments if needed and ensure the exported symbol name
remains FederatedDirectivesData.
In `@composition/src/schema-building/utils.ts`:
- Around line 211-216: Replace the loose equality check between printed
default-value strings with a strict equality check: in the block that computes
existingDefaultValueString = print(existingData.defaultValue) and
incomingDefaultValueString = print(incomingData.defaultValue), change the
comparison from `==` to `===` so the early-return path in the function uses
strict equality (`existingDefaultValueString === incomingDefaultValueString`)
consistent with project guidelines.
In `@composition/src/subgraph/types/params.ts`:
- Around line 4-7: Replace the exported type alias
InternalSubgraphFromNormalizationParams with an exported interface of the same
name describing the same properties (normalization: NormalizationSuccess;
subgraphName: SubgraphName) so the object shape uses an interface instead of a
type alias; keep the same exported identifier and referenced types
(NormalizationSuccess, SubgraphName) so all callers remain compatible.
In `@composition/src/utils/params.ts`:
- Around line 16-19: Replace the type alias with an interface for the
object-shaped params: change the declaration of MergeSetValueMapParams to an
interface that declares source: Map<K, Set<V>> and target: Map<K, Set<V>> so it
follows the repo TypeScript guideline to use interfaces for object-shaped
contracts; keep the generic parameters <K, V> and the property names exactly as
in MergeSetValueMapParams to preserve usage compatibility.
In `@composition/src/v1/federation/federation-factory.ts`:
- Around line 1507-1549: Remove the large commented-out implementation inside
upsertPersistedDirectiveDefinitionData(): delete the dead code block (the
commented logic referencing potentialPersistedDirectiveDefinitionDataByName,
setMutualExecutableLocations, upsertInputValueData, setLongestDescription,
addIterableToSet, etc.) and either remove the empty function if it's unused or
replace it with a single-line TODO/issue-link comment explaining why
persisted-directive logic was retired; ensure no removed symbols are referenced
elsewhere or update callsites accordingly.
In `@composition/src/v1/normalization/batch-normalization/types/params.ts`:
- Around line 3-7: Replace the exported type alias HandleOverridesParams with an
exported interface of the same name and identical properties
(originalTypeNameByRenamedTypeName,
overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the
object contract is defined as an interface (preserve the Map/Set types and their
names) to match the repo style and enable easier future extension; update any
local references if needed but keep the exported symbol name unchanged.
In `@composition/src/validation/types/params.ts`:
- Around line 6-26: Replace the three exported object-shape type aliases
IsArgumentValueValidParams, ValidateCustomDirectiveParams, and
ValidateDirectivesParams with exported interfaces of the same names and
identical member signatures (argumentValue, parentDefinitionDataByTypeName,
typeNode for IsArgumentValueValidParams; argumentDataByName, directiveNode,
parentDefinitionDataByTypeName, requiredArgumentNames for
ValidateCustomDirectiveParams; data, directiveCoords, directiveDefinitionData,
directiveNodes, parentDefinitionDataByTypeName for ValidateDirectivesParams) so
the module follows the repo convention of using interfaces for parameter-bag
objects; keep all property types and export names unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86097bfb-346d-4a7f-9e62-613019c5b147
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
.github/actions/node/action.yaml.github/workflows/cli-ci.yamlcomposition-go/index.global.jscomposition/package.jsoncomposition/src/directive-definition-data/directive-definition-data.tscomposition/src/directive-definition-data/types/params.tscomposition/src/directive-definition-data/types/types.tscomposition/src/directive-definition-data/utils.tscomposition/src/errors/errors.tscomposition/src/errors/types/params.tscomposition/src/errors/types/types.tscomposition/src/federation/federation.tscomposition/src/federation/types/params.tscomposition/src/federation/types/results.tscomposition/src/federation/types/types.tscomposition/src/index.tscomposition/src/normalization/normalization.tscomposition/src/normalization/types.tscomposition/src/schema-building/ast.tscomposition/src/schema-building/params.tscomposition/src/schema-building/types/params.tscomposition/src/schema-building/types/results.tscomposition/src/schema-building/types/types.tscomposition/src/schema-building/utils.tscomposition/src/subgraph/types.tscomposition/src/subgraph/types/params.tscomposition/src/subgraph/utils.tscomposition/src/types/results.tscomposition/src/types/types.tscomposition/src/utils/params.tscomposition/src/utils/string-constants.tscomposition/src/utils/utils.tscomposition/src/v1/constants/strings.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/types/params.tscomposition/src/v1/federation/utils.tscomposition/src/v1/federation/walkers.tscomposition/src/v1/normalization/batch-normalization/batch-normalizer.tscomposition/src/v1/normalization/batch-normalization/types/params.tscomposition/src/v1/normalization/batch-normalization/types/results.tscomposition/src/v1/normalization/batch-normalization/types/types.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/params.tscomposition/src/v1/normalization/types/results.tscomposition/src/v1/normalization/types/types.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/normalization/walkers.tscomposition/src/v1/schema-building/type-merging.tscomposition/src/v1/utils/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/src/validation/types/params.tscomposition/src/validation/types/results.tscomposition/src/validation/validation.tscomposition/tests/v1/directives/compose-directive.test.tscomposition/tests/v1/directives/override.test.tscomposition/tests/v1/events.test.tscomposition/tests/v1/federation-factory.test.tscomposition/tests/v1/router-configuration.test.tspnpm-workspace.yaml
💤 Files with no reviewable changes (2)
- composition/src/schema-building/params.ts
- composition/src/v1/normalization/directive-definition-data.ts
Contributor
Author
|
@CodeRabbit stop |
Contributor
|
🧠 Learnings used✅ Actions performedReviews paused. |
This comment has been minimized.
This comment has been minimized.
comatory
force-pushed
the
ondrej/eng-3812-implement-composedirective-directive
branch
from
0e2f3d2 to
d248ab0
Compare
comatory
force-pushed
the
ondrej/eng-3812-implement-composedirective-directive
branch
from
d248ab0 to
309328f
Compare
comatory
deleted the
ondrej/eng-3812-implement-composedirective-directive
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test changes related to #2805
Summary by CodeRabbit
New Features
Chores
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.