Skip to content

Commit 9ce5acc

Browse files
committed
Fix review findings: extract isScalarLikeType, safe union type assertion
- Extract isScalarLikeType() to type-utils.ts shared utility, replacing duplicated inline check in union-type.tsx - Replace unsafe `as Union` casts in union-type tests with assertUnionResult() helper that provides a clear error if the mutation returns a Model
1 parent ddd0ed8 commit 9ce5acc

3 files changed

Lines changed: 27 additions & 14 deletions

File tree

packages/graphql/src/components/types/union-type.tsx

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,13 @@
1-
import { type Type, type Union, getDoc } from "@typespec/compiler";
1+
import { type Union, getDoc } from "@typespec/compiler";
22
import * as gql from "@alloy-js/graphql";
33
import { useTsp } from "@typespec/emitter-framework";
4-
import { getUnionName, toTypeName } from "../../lib/type-utils.js";
4+
import { getUnionName, isScalarLikeType, toTypeName } from "../../lib/type-utils.js";
55

66
export interface UnionTypeProps {
77
/** The union type to render */
88
type: Union;
99
}
1010

11-
/**
12-
* Check if a type is a scalar (built-in or custom)
13-
*/
14-
function isScalarType(type: Type): boolean {
15-
return type.kind === "Scalar" || type.kind === "Intrinsic";
16-
}
17-
1811
/**
1912
* Renders a GraphQL union type declaration
2013
* Scalars are wrapped in object types since GraphQL unions can only contain object types
@@ -32,7 +25,7 @@ export function UnionType(props: UnionTypeProps) {
3225
const variantName =
3326
typeof variant.name === "string" ? variant.name : String(variant.name);
3427

35-
if (isScalarType(variant.type)) {
28+
if (isScalarLikeType(variant.type)) {
3629
// Reference the wrapper type for scalars (created by mutation engine)
3730
// Include union name to match wrapper model naming convention
3831
return toTypeName(name) + toTypeName(variantName) + "UnionVariant";

packages/graphql/src/lib/type-utils.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,11 @@ function getTemplateStringInternal(
318318
return args.length > 0 ? args.map(toTypeName).join(options.conjunction) : "";
319319
}
320320

321+
/** Check if a type is a scalar (built-in or custom) or an intrinsic type like `unknown`. */
322+
export function isScalarLikeType(type: Type): boolean {
323+
return type.kind === "Scalar" || type.kind === "Intrinsic";
324+
}
325+
321326
/** Check if a model should be emitted as a GraphQL object type (not an array, record, or never). */
322327
export function isTrueModel(model: Model): boolean {
323328
return !(

packages/graphql/test/components/union-type.test.tsx

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,22 @@ import { describe, expect, it, beforeEach } from "vitest";
55
import { UnionType } from "../../src/components/types/index.js";
66
import {
77
createGraphQLMutationEngine,
8+
type GraphQLUnionMutation,
89
GraphQLTypeContext,
910
} from "../../src/mutation-engine/index.js";
1011
import { Tester } from "../test-host.js";
1112
import { renderComponentToSDL } from "./component-test-utils.js";
1213

14+
/** Assert that an output-context union mutation produced a Union (not a Model). */
15+
function assertUnionResult(mutation: GraphQLUnionMutation): Union {
16+
if (mutation.mutatedType.kind !== "Union") {
17+
throw new Error(
18+
`Expected Union from output-context mutation, got ${mutation.mutatedType.kind}`,
19+
);
20+
}
21+
return mutation.mutatedType;
22+
}
23+
1324
describe("UnionType component", () => {
1425
let tester: Awaited<ReturnType<typeof Tester.createInstance>>;
1526
beforeEach(async () => {
@@ -27,6 +38,7 @@ describe("UnionType component", () => {
2738

2839
const engine = createGraphQLMutationEngine(tester.program);
2940
const mutation = engine.mutateUnion(Pet, GraphQLTypeContext.Output);
41+
const mutatedUnion = assertUnionResult(mutation);
3042

3143
// Union members must be registered in the schema for buildSchema to resolve them
3244
const sdl = renderComponentToSDL(
@@ -38,7 +50,7 @@ describe("UnionType component", () => {
3850
<gql.ObjectType name="Dog">
3951
<gql.Field name="breed" type={gql.String} nonNull />
4052
</gql.ObjectType>
41-
<UnionType type={mutation.mutatedType as Union} />
53+
<UnionType type={mutatedUnion} />
4254
</>,
4355
);
4456

@@ -59,6 +71,7 @@ describe("UnionType component", () => {
5971

6072
const engine = createGraphQLMutationEngine(tester.program);
6173
const mutation = engine.mutateUnion(Result, GraphQLTypeContext.Output);
74+
const mutatedUnion = assertUnionResult(mutation);
6275

6376
const sdl = renderComponentToSDL(
6477
tester.program,
@@ -69,7 +82,7 @@ describe("UnionType component", () => {
6982
<gql.ObjectType name="Failure">
7083
<gql.Field name="message" type={gql.String} nonNull />
7184
</gql.ObjectType>
72-
<UnionType type={mutation.mutatedType as Union} />
85+
<UnionType type={mutatedUnion} />
7386
</>,
7487
);
7588

@@ -89,6 +102,7 @@ describe("UnionType component", () => {
89102

90103
const engine = createGraphQLMutationEngine(tester.program);
91104
const mutation = engine.mutateUnion(Shape, GraphQLTypeContext.Output);
105+
const mutatedUnion = assertUnionResult(mutation);
92106

93107
const sdl = renderComponentToSDL(
94108
tester.program,
@@ -102,7 +116,7 @@ describe("UnionType component", () => {
102116
<gql.ObjectType name="Triangle">
103117
<gql.Field name="base" type={gql.Float} nonNull />
104118
</gql.ObjectType>
105-
<UnionType type={mutation.mutatedType as Union} />
119+
<UnionType type={mutatedUnion} />
106120
</>,
107121
);
108122

@@ -122,6 +136,7 @@ describe("UnionType component", () => {
122136

123137
const engine = createGraphQLMutationEngine(tester.program);
124138
const mutation = engine.mutateUnion(Mixed, GraphQLTypeContext.Output);
139+
const mutatedUnion = assertUnionResult(mutation);
125140

126141
// Register Cat and the wrapper type that the union will reference
127142
const sdl = renderComponentToSDL(
@@ -133,7 +148,7 @@ describe("UnionType component", () => {
133148
<gql.ObjectType name="MixedTextUnionVariant">
134149
<gql.Field name="value" type={gql.String} nonNull />
135150
</gql.ObjectType>
136-
<UnionType type={mutation.mutatedType as Union} />
151+
<UnionType type={mutatedUnion} />
137152
</>,
138153
);
139154

0 commit comments

Comments
 (0)