From 0acd33aeed7e423c4b53595940fc036d633d82ff Mon Sep 17 00:00:00 2001 From: naman-contentstack Date: Mon, 31 Aug 2026 14:19:55 +0530 Subject: [PATCH] fix(tsgen): stop modular block interfaces colliding with other generated names REST-mode type generation emitted two interfaces with the same name when a modular blocks field's UID matched the UID of a content type, producing a .d.ts that does not compile: export interface Form { heading?: string } // content type `form` export interface Form { heading: { ... } } // blocks field `form` TypeScript then reports TS2687 and TS2717 on the merged members. Block interface names were de-duplicated only against other block names, so they were blind to content types, global fields and the builtin interfaces. Content types are also generated one at a time, so the factory could not see a content type it had not reached yet. Route every block name through a single registry, seeded up front with the builtin names and with every top-level name in the batch: - interfaceNameForUid() is now the one place a UID becomes a name. - collectBuiltinInterfaceNames() reads the builtin names from stack/builtins.ts rather than restating them, so the list cannot drift, and passes the real emission flags so names that are not emitted are not reserved. - generateTSFromContentTypes reserves all top-level names before the loop, making the fix independent of content type ordering. - The prefix is normalised once at that boundary so the builtins, the reserved names and the generated interfaces agree. Top-level interfaces are never renamed: customers import those by name. Only the derived block interface is suffixed, and the suffix counter is deliberately left shared so existing output is unchanged - a stack that produces Card2 today still produces Card2, not Card1. A rename is now reported through the logger, matching every other name-mangling path in the file, so a customer does not meet it first as a "Cannot find name" error in their own build. Group fields are unaffected: they are emitted inline and claim no name, so they cannot collide. The ticket describes the trigger as a group field; the customer's generated output confirms it is a modular blocks field. Known gap, unchanged by this commit and tracked separately: a content type whose UID maps to a builtin name still emits a duplicate. Both sides are top-level, so neither can be renamed without breaking imports. Verified byte-identical to the previous behaviour. Fixes DX-10385 Co-Authored-By: Claude Opus 5 (1M context) --- src/constants/messages.ts | 2 + src/generateTS/factory.ts | 106 ++++++++++-- src/generateTS/index.ts | 21 ++- tests/unit/tsgen/name-collisions.ct.js | 44 +++++ tests/unit/tsgen/name-collisions.test.ts | 200 +++++++++++++++++++++++ 5 files changed, 352 insertions(+), 21 deletions(-) create mode 100644 tests/unit/tsgen/name-collisions.ct.js create mode 100644 tests/unit/tsgen/name-collisions.test.ts diff --git a/src/constants/messages.ts b/src/constants/messages.ts index 8bfdf77..e2c574e 100644 --- a/src/constants/messages.ts +++ b/src/constants/messages.ts @@ -40,6 +40,8 @@ export const ERROR_MESSAGES = { `Skipped global field "${uid}": ${reason}`, SKIPPED_GLOBAL_FIELD_NO_SCHEMA: (uid: string, reason: string) => `Skipped global field "${uid}": ${reason}. Did you forget to include it?`, + RENAMED_BLOCK_INTERFACE: (from: string, to: string) => + `Renamed modular block interface "${from}" to "${to}": that name is already used by another generated interface.`, SKIPPED_REFERENCE: (reference: string, reason: string) => `Skipped reference to content type "${reference}": ${reason}`, diff --git a/src/generateTS/factory.ts b/src/generateTS/factory.ts index d76f942..628a08b 100644 --- a/src/generateTS/factory.ts +++ b/src/generateTS/factory.ts @@ -11,6 +11,7 @@ import { throwNumericIdentifierValidationError, } from "./shared/utils"; import { ERROR_MESSAGES } from "../constants"; +import { defaultInterfaces } from "./stack/builtins"; export function hasPrefixedNaming(prefix: string | undefined): boolean { return typeof prefix === "string" && prefix.trim().length > 0; @@ -24,6 +25,55 @@ export function composePrefixedInterfaceName( return trimmed + _.upperFirst(_.camelCase(uid)); } +/** + * Every interface/type name that stack/builtins.ts will actually emit for this run, read + * from that module rather than restated here — a hand-copied list silently drifts as + * builtins are added, and a name missing from it collides in the generated output with + * no warning. + * + * The emission flags must be passed through accurately rather than all-enabled. Reserving + * a name that is not emitted is not harmless: the block that wanted it gets renamed, and + * it also consumes the shared suffix counter, shifting the suffix of every later + * collision in the batch. Both would rename interfaces that compile today. + * + * The JSON RTE flag is passed as true deliberately. The only name it adds is the JSON + * rich-text node interface, which a UID can never produce: a name is the upper-cased + * camel case of its UID, and that cannot yield an all-caps acronym prefix. The two + * live-preview helper names are unreachable for the same reason, so reserving any of + * the three can never rename anything. + */ +function collectBuiltinInterfaceNames( + prefix: string, + systemFields: boolean, + isEditableTags: boolean, + includeReferencedEntry: boolean, +): string[] { + const declarations = defaultInterfaces( + prefix, + systemFields, + isEditableTags, + true, + includeReferencedEntry, + ).join("\n"); + + // Deliberately an exec loop rather than matchAll: tsconfig targets es2017, and + // String.prototype.matchAll is es2020. It runs fine on Node, but it does not type-check. + const names: string[] = []; + const pattern = /(?:interface|type)\s+(\w+)/g; + let match: RegExpExecArray | null; + while ((match = pattern.exec(declarations)) !== null) { + names.push(match[1]); + } + return names; +} +export function interfaceNameForUid(uid: string, prefix: string): string { + const trimmed = typeof prefix === "string" ? prefix.trim() : ""; + if (!trimmed && isNumericIdentifier(uid)) { + return `InvalidInterface_${uid}`; + } + return composePrefixedInterfaceName(uid, trimmed); +} + export type TSGenOptions = { docgen: DocumentationGenerator; naming?: { @@ -33,6 +83,8 @@ export type TSGenOptions = { isEditableTags?: boolean; includeReferencedEntry?: boolean; logger?: Logger; + /** Interface names already claimed by the batch — see generateTSFromContentTypes. */ + reservedNames?: string[]; }; export type TSGenResult = { @@ -59,10 +111,6 @@ type GlobalFieldCache = { [prop: string]: { definition: string }; }; -type ModularBlockCache = { - [prop: string]: string; -}; - enum TypeFlags { BuiltinJS = 1 << 0, BuiltinCS = 1 << 1, @@ -100,11 +148,9 @@ export default function (userOptions: TSGenOptions) { const visitedGlobalFields = new Set(); const visitedContentTypes = new Set(); const cachedGlobalFields: GlobalFieldCache = {}; - const cachedModularBlocks: ModularBlockCache = {}; const modularBlockInterfaces = new Set(); const uniqueBlockInterfaces = new Set(); const blockInterfacesKeyToName: { [key: string]: string } = {}; - let counter = 1; const skippedFields: Array<{ uid: string; path: string; reason: string }> = []; const skippedBlocks: Array<{ uid: string; path: string; reason: string }> = @@ -115,6 +161,40 @@ export default function (userOptions: TSGenOptions) { ? options.naming.prefix.trim() : ""; + // Every interface name already claimed: the builtins, plus every top-level content + // type and global field in this batch (seeded by generateTSFromContentTypes, which + // knows the whole batch — the factory itself only ever sees one content type at a + // time). Top-level names are never reallocated; only derived block names are. + const usedInterfaceNames = new Set([ + ...collectBuiltinInterfaceNames( + trimmedNamingPrefix, + Boolean(options.systemFields), + Boolean(options.isEditableTags), + Boolean(options.includeReferencedEntry), + ), + ...(options.reservedNames ?? []), + ]); + + // Shared across all block-name collisions, deliberately: this reproduces the numbering + // the generator has always produced, so no interface that compiles today is renamed. + // A per-name counter would be tidier but would turn an existing `Card2` into `Card1`. + let counter = 1; + + function reserveInterfaceName(baseName: string): string { + let candidate = baseName; + while (usedInterfaceNames.has(candidate)) { + candidate = `${candidate}${counter}`; + counter++; + } + if (candidate !== baseName) { + // Every other name-mangling path in this file reports itself; a silent rename + // leaves the customer with "Cannot find name 'Form'" and nothing to explain it. + logger?.warn(ERROR_MESSAGES.RENAMED_BLOCK_INTERFACE(baseName, candidate)); + } + usedInterfaceNames.add(candidate); + return candidate; + } + // Collect numeric identifier errors instead of throwing immediately const numericIdentifierErrors: Array<{ uid: string; @@ -180,13 +260,7 @@ export default function (userOptions: TSGenOptions) { } function name_type(uid: string) { - if (trimmedNamingPrefix) { - return composePrefixedInterfaceName(uid, trimmedNamingPrefix); - } - if (isNumericIdentifier(uid)) { - return `InvalidInterface_${uid}`; - } - return composePrefixedInterfaceName(uid, ""); + return interfaceNameForUid(uid, trimmedNamingPrefix); } function define_interface( @@ -488,10 +562,7 @@ export default function (userOptions: TSGenOptions) { uniqueBlockInterfaces.add(modularBlockSignature); - while (cachedModularBlocks[modularBlockInterfaceName]) { - modularBlockInterfaceName = `${modularBlockInterfaceName}${counter}`; - counter++; - } + modularBlockInterfaceName = reserveInterfaceName(modularBlockInterfaceName); const modularBlockInterfaceDefinition = [ `export interface ${modularBlockInterfaceName}${options.systemFields ? ` extends ${trimmedNamingPrefix}SystemFields` : ""} {`, @@ -501,7 +572,6 @@ export default function (userOptions: TSGenOptions) { // Store or track the generated block interface for later use modularBlockInterfaces.add(modularBlockInterfaceDefinition); - cachedModularBlocks[modularBlockInterfaceName] = modularBlockSignature; blockInterfacesKeyToName[modularBlockSignature] = modularBlockInterfaceName; // Wrap with ModularBlocks type to add _metadata support only when systemFields is enabled diff --git a/src/generateTS/index.ts b/src/generateTS/index.ts index d32f187..20de4b4 100644 --- a/src/generateTS/index.ts +++ b/src/generateTS/index.ts @@ -6,7 +6,7 @@ import { GenerateTS, GenerateTSFromContentTypes } from "../types"; import { DocumentationGenerator } from "./docgen/doc"; import JSDocumentationGenerator from "./docgen/jsdoc"; import NullDocumentationGenerator from "./docgen/nulldoc"; -import tsgenFactory from "./factory"; +import tsgenFactory, { interfaceNameForUid } from "./factory"; import { defaultInterfaces } from "./stack/builtins"; import { format } from "../format/index"; import { ContentType } from "../types/schema"; @@ -139,13 +139,28 @@ export const generateTSFromContentTypes = async ({ const globalFields = new Set(); const definitions = []; + // Normalise once, here, so that the builtins, the reserved names and the generated + // interfaces all agree on the prefix. They used to disagree: `defaultInterfaces` got + // the raw value while the factory trimmed it, so a prefix of `null` emitted + // `nullFile` and a prefix of " CS " emitted ` CS File` against a reserved + // `CSFile`. + const normalizedPrefix = (prefix ?? "").trim(); + + // Every top-level interface name in this batch, claimed before generation starts. + // Content types are visited one at a time, so without this the factory cannot know + // about a content type it has not reached yet (DX-10385). + const reservedNames = contentTypes.map((contentType) => + interfaceNameForUid(contentType.uid, normalizedPrefix) + ); + const tsgen = tsgenFactory({ docgen, - naming: { prefix }, + naming: { prefix: normalizedPrefix }, systemFields, isEditableTags, includeReferencedEntry, logger, + reservedNames, }); for (const contentType of contentTypes) { const tsgenResult = tsgen(contentType); @@ -169,7 +184,7 @@ export const generateTSFromContentTypes = async ({ const output = await format( [ defaultInterfaces( - prefix, + normalizedPrefix, systemFields, isEditableTags, hasJsonField, diff --git a/tests/unit/tsgen/name-collisions.ct.js b/tests/unit/tsgen/name-collisions.ct.js new file mode 100644 index 0000000..fbd9d80 --- /dev/null +++ b/tests/unit/tsgen/name-collisions.ct.js @@ -0,0 +1,44 @@ +const text = (uid) => ({ uid, data_type: "text", multiple: false }); + +// A content type whose modular-blocks field is named `file`, colliding with the +// built-in `File` interface. +const blockVsBuiltin = { + uid: "page", + title: "Page", + schema_type: "content_type", + schema: [ + text("title"), + { + uid: "file", + data_type: "blocks", + multiple: true, + blocks: [{ uid: "hero", title: "Hero", schema: [text("label")] }], + }, + ], +}; + +// DX-10385: content type `form`, plus content type `form_basic` whose modular-blocks +// field is also UID'd `form`. +const formCT = { + uid: "form", + title: "Form", + schema_type: "content_type", + schema: [text("title"), text("heading")], +}; + +const formBasicCT = { + uid: "form_basic", + title: "Form Basic", + schema_type: "content_type", + schema: [ + text("title"), + { + uid: "form", + data_type: "blocks", + multiple: true, + blocks: [{ uid: "heading", title: "Heading", schema: [text("label")] }], + }, + ], +}; + +module.exports = { blockVsBuiltin, formCT, formBasicCT }; diff --git a/tests/unit/tsgen/name-collisions.test.ts b/tests/unit/tsgen/name-collisions.test.ts new file mode 100644 index 0000000..6e51922 --- /dev/null +++ b/tests/unit/tsgen/name-collisions.test.ts @@ -0,0 +1,200 @@ +const testData = require("./name-collisions.ct"); + +import NullDocumentationGenerator from "../../../src/generateTS/docgen/nulldoc"; +import tsgenFactory from "../../../src/generateTS/factory"; +import { generateTSFromContentTypes } from "../../../src/generateTS/index"; + +const interfaceNames = (output: string) => + [...output.matchAll(/export interface (\w+)/g)].map((m) => m[1]); + +describe("interface name collisions", () => { + test("a modular block named `file` does not reuse the builtin File name", () => { + const tsgen = tsgenFactory({ docgen: new NullDocumentationGenerator() }); + const result = tsgen(testData.blockVsBuiltin); + + // The block interface must not be called `File` — that name is taken by the + // builtin emitted in stack/builtins.ts. + expect(result.definition).not.toMatch(/export interface File\b/); + expect(result.definition).toMatch(/export interface File1\b/); + }); +}); + +describe("DX-10385: content type UID vs modular block UID", () => { + test("emits no duplicate interface names", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [testData.formCT, testData.formBasicCT], + prefix: "", + includeDocumentation: false, + }); + + const names = interfaceNames(output); + const duplicates = names.filter((n, i) => names.indexOf(n) !== i); + expect(duplicates).toEqual([]); + }); + + test("the content type keeps the `Form` name; the block is renamed", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [testData.formCT, testData.formBasicCT], + prefix: "", + includeDocumentation: false, + }); + + // The top-level content type must keep its name — customers import it. + expect(output).toMatch(/export interface Form\s*\{[^}]*heading\?: string/); + // The derived block interface takes the suffixed name. + expect(output).toMatch(/export interface Form1\b/); + expect(output).toMatch(/form\?: Form1\[\]/); + }); + + test("order does not matter — the colliding content type may come second", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [testData.formBasicCT, testData.formCT], + prefix: "", + includeDocumentation: false, + }); + + const names = interfaceNames(output); + const duplicates = names.filter((n, i) => names.indexOf(n) !== i); + expect(duplicates).toEqual([]); + }); +}); + +const text = (uid: string) => ({ uid, data_type: "text", multiple: false }); +const ct = (uid: string, schema: any[]) => ({ + uid, + title: uid, + schema_type: "content_type", + schema, +}); +const blocksField = (uid: string, innerField: string) => ({ + uid, + data_type: "blocks", + multiple: true, + blocks: [{ uid: "banner", title: "Banner", schema: [text(innerField)] }], +}); + +describe("builtin names are reserved, including the unprefixed ones", () => { + // BuildTuple / TuplePrefixes / MaxTuple are emitted by stack/builtins.ts WITHOUT the + // naming prefix, so they are easy to miss when the reserved list is maintained by + // hand. The names are now read from builtins.ts so that this cannot drift again. + it.each(["build_tuple", "tuple_prefixes", "max_tuple", "file", "link"])( + "a modular block named `%s` does not reuse the builtin name", + async (blockFieldUid) => { + const output = await generateTSFromContentTypes({ + contentTypes: [ + ct("page", [text("title"), blocksField(blockFieldUid, "label")]), + ] as any, + prefix: "", + includeDocumentation: false, + }); + + const declared = [ + ...output.matchAll(/(?:export\s+)?(?:interface|type)\s+(\w+)/g), + ].map((m) => m[1]); + const duplicates = declared.filter((n, i) => declared.indexOf(n) !== i); + expect(duplicates).toEqual([]); + } + ); +}); + +describe("names that already compile are never renamed", () => { + // Stacks with several block-vs-block collisions generate valid types today. The + // suffix counter is shared across all collisions, so the second colliding base name + // starts at 2 and `Card1` is never produced. That numbering is arbitrary, but it is + // what has shipped — switching to a per-name counter would rename a live interface. + test("the shared suffix counter is preserved (Card2, not Card1)", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [ + ct("page_a", [text("title"), blocksField("hero", "alpha")]), + ct("page_b", [text("title"), blocksField("hero", "beta")]), + ct("page_c", [text("title"), blocksField("card", "gamma")]), + ct("page_d", [text("title"), blocksField("card", "delta")]), + ] as any, + prefix: "", + includeDocumentation: false, + }); + + expect(interfaceNames(output)).toEqual([ + "PublishDetails", + "File", + "Link", + "Taxonomy", + "Hero", + "PageA", + "Hero1", + "PageB", + "Card", + "PageC", + "Card2", + "PageD", + ]); + }); + + test("a null prefix produces valid output, not `nullFile` builtins", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [ct("article", [text("title")])] as any, + prefix: null as any, + includeDocumentation: false, + }); + + expect(output).toMatch(/export interface Article\b/); + // The prefix is normalised at the boundary, so the builtins are not emitted as + // `nullFile` / `nullLink` against references to a `File` that was never declared. + expect(output).not.toMatch(/null(File|Link|Taxonomy|PublishDetails)/); + expect(output).toMatch(/export interface File\b/); + }); + + test("a padded prefix is trimmed consistently across builtins and interfaces", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [ct("article", [text("title")])] as any, + prefix: " CS ", + includeDocumentation: false, + }); + + expect(output).toMatch(/export interface CSArticle\b/); + expect(output).toMatch(/export interface CSFile\b/); + expect(output).not.toMatch(/interface\s+\s+CS/); + }); +}); + +describe("builtins that are not emitted are not reserved", () => { + // Reserving a name that will not be emitted is not free: the block that wanted it is + // renamed, AND it consumes the shared suffix counter, shifting every later collision + // in the batch. Both rename interfaces that compile today. + test("a block named `system_fields` keeps its name when systemFields is off, and does not shift later suffixes", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [ + ct("page_a", [text("title"), blocksField("system_fields", "alpha")]), + ct("page_b", [text("title"), blocksField("hero", "beta")]), + ct("page_c", [text("title"), blocksField("hero", "gamma")]), + ] as any, + prefix: "", + systemFields: false, + includeDocumentation: false, + }); + + // SystemFields is not emitted when systemFields is false, so the block may use it. + expect(output).toMatch(/export interface SystemFields\b/); + expect(output).not.toMatch(/export interface SystemFields1\b/); + // ...and the untouched counter means the later `hero` collision is still Hero1. + expect(output).toMatch(/export interface Hero1\b/); + expect(output).not.toMatch(/export interface Hero2\b/); + }); + + test("a block named `system_fields` is renamed when systemFields is on", async () => { + const output = await generateTSFromContentTypes({ + contentTypes: [ + ct("page_a", [text("title"), blocksField("system_fields", "alpha")]), + ] as any, + prefix: "", + systemFields: true, + includeDocumentation: false, + }); + + const declared = [ + ...output.matchAll(/(?:export\s+)?(?:interface|type)\s+(\w+)/g), + ].map((m) => m[1]); + expect(declared.filter((n, i) => declared.indexOf(n) !== i)).toEqual([]); + }); +}); +