Skip to content

fix(seo-graph-core): return GraphEntity from piece builders#55

Open
schlessera wants to merge 2 commits into
jdevalk:mainfrom
schlessera:fix/seo-graph-core-builders-return-graphentity
Open

fix(seo-graph-core): return GraphEntity from piece builders#55
schlessera wants to merge 2 commits into
jdevalk:mainfrom
schlessera:fix/seo-graph-core-builders-return-graphentity

Conversation

@schlessera

Copy link
Copy Markdown

Stacked on #54. Until that merges, this PR shows both commits; the diff here reduces to the GraphEntity change once #54 lands. Per-PR review of just this change: compare the second commit.

Problem

assembleGraph<T extends GraphEntity>(pieces: readonly T[]) requires each piece to satisfy GraphEntity, whose @type is required:

// types.ts
export interface GraphEntity {
    '@type': string | readonly string[];
    '@id'?: string;
    [key: string]: unknown;
}

But every builder is declared to return Record<string, unknown>, which has no @type. So the pattern documented in the core README and AGENTS.md —

assembleGraph([buildWebSite(...), buildArticle(...)]);

— fails tsc / astro check under strict mode:

Property '@type' is missing in type 'Record<string, unknown>' but required in type 'GraphEntity'.

Callers must cast as GraphEntity[]. The same bite hits the aggregate / createSchemaEndpoint mapper, whose type is already (entry) => ReadonlyArray<GraphEntity> — builder outputs don't satisfy it without a cast.

Fix

Widen the return type (and the local piece) of all eight builders to GraphEntity:

buildWebSite, buildWebPage, buildArticle, buildBreadcrumbList, buildImageObject, buildVideoObject, buildSiteNavigationElement, buildPiece.

Every builder already constructs its object with a literal @type, so this is a pure type change — no runtime difference. buildPiece's public overloads already require @type on the input, so its result is a GraphEntity; the implementation signature stays wider and carries one local as GraphEntity (documented inline).

No changes needed in astro-seo-graphaggregate already expects GraphEntity; this just lets callers satisfy it without a cast.

Test

Added an assembleGraph test that composes real builder outputs directly (no cast). Because pnpm typecheck covers the test files, it doubles as a compile-time regression guard — it would not have type-checked under the old Record<string, unknown> returns.

Verification

pnpm build && pnpm typecheck && pnpm test && pnpm format:check all green locally (core 60 tests, +1 new; 475 total).

changeset: minor (widens public return types; non-breaking).

ArticleInput.isPartOf was typed as a single `Reference`, but the shipped
AGENTS.md "Personal blog" recipe links a posting to both its WebPage and
the Blog via `isPartOf: [{ '@id': webPage }, { '@id': blog }]`. The
builder already passes the value through verbatim (`isPartOf:
input.isPartOf`), so the array worked at runtime — but the type rejected
it, forcing an `as` cast.

Widen the input type to `Reference | Reference[]`. Pure type change; no
runtime difference. Add a test asserting the array is emitted verbatim.
assembleGraph<T extends GraphEntity>(pieces) requires each piece to be a
GraphEntity, whose `@type` is required. Every builder was declared to
return `Record<string, unknown>`, which lacks `@type` — so the documented
pattern `assembleGraph([buildWebSite(...), buildArticle(...)])` failed tsc
/ astro check under strict mode and forced callers to cast `as
GraphEntity[]` (and the aggregate / createSchemaEndpoint mapper type,
already `=> ReadonlyArray<GraphEntity>`, couldn't be satisfied without a
cast either).

Each builder already constructs an object with a literal `@type`, so the
return type (and the local `piece`) is widened to `GraphEntity` with no
runtime change. buildPiece's public overloads already require `@type` on
the input, so its result is a GraphEntity too (one internal cast in the
impl signature, which is intentionally wider).

Add an assembleGraph test that composes builder outputs directly with no
cast — it doubles as a tsc-level regression guard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant