fix: unexpected unknown type for api docs format type#1663
fix: unexpected unknown type for api docs format type#1663danielroe merged 3 commits intonpmx-dev:mainfrom
unknown type for api docs format type#1663Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
RYGRIT
changed the title
unknown type for docs format typeunknown type for api docs format type
📝 WalkthroughWalkthroughThis pull request refactors the type formatting system in Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![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 (1)
server/utils/docs/format.ts (1)
124-137:typeLiteraloutput is incomplete for callable/indexable object types.The current formatter only emits properties and methods.
callSignaturesandindexSignaturesare silently dropped, which can still produce inaccurate docs for valid type literals.Proposed enhancement
function formatTypeLiteralType(lit: NonNullable<TsType['typeLiteral']>): string { const parts: string[] = [] for (const prop of lit.properties) { const opt = prop.optional ? '?' : '' const ro = prop.readonly ? 'readonly ' : '' parts.push(`${ro}${prop.name}${opt}: ${formatType(prop.tsType) || 'unknown'}`) } for (const method of lit.methods) { const params = method.params?.map(p => formatParam(p)).join(', ') || '' const ret = formatType(method.returnType) || 'void' parts.push(`${method.name}(${params}): ${ret}`) } + for (const callSig of lit.callSignatures) { + const params = callSig.params?.map(p => formatParam(p)).join(', ') || '' + const ret = formatType(callSig.tsType) || 'void' + parts.push(`(${params}): ${ret}`) + } + for (const indexSig of lit.indexSignatures) { + const params = indexSig.params.map(p => formatParam(p)).join(', ') + const ret = formatType(indexSig.tsType) || 'unknown' + parts.push(`[${params}]: ${ret}`) + } return `{ ${parts.join('; ')} }` }
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server/utils/docs/format.tsshared/types/deno-doc.tstest/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-client-message.jsontest/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-client.jsontest/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-manager-events.jsontest/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-pick-type.jsontest/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-send-to-shard.jsontest/unit/server/utils/docs/format.spec.ts
| function formatFnOrConstructorType(fn: NonNullable<TsType['fnOrConstructor']>): string { | ||
| const typeParams = fn.typeParams?.map(t => t.name).join(', ') | ||
| const typeParamsStr = typeParams ? `<${typeParams}>` : '' | ||
| const params = fn.params.map(p => formatParam(p)).join(', ') | ||
| const ret = formatType(fn.tsType) || 'void' | ||
| return `${typeParamsStr}(${params}) => ${ret}` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/utils/docs/format.ts | head -150 | tail -50Repository: npmx-dev/npmx.dev
Length of output: 2493
🏁 Script executed:
rg -n "fnOrConstructor|TsType" server/utils/docs/format.ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 581
🏁 Script executed:
rg -n "interface.*TsType|type.*TsType" server/utils/docs/ -A 30Repository: npmx-dev/npmx.dev
Length of output: 5460
🏁 Script executed:
fd -t f "types" --type f -path "*shared/types*" | head -20Repository: npmx-dev/npmx.dev
Length of output: 229
🏁 Script executed:
find . -type f -name "*deno-doc*" -o -name "*types*" | grep -i "shared\|types" | head -20Repository: npmx-dev/npmx.dev
Length of output: 340
🏁 Script executed:
cat -n shared/types/deno-doc.ts | grep -A 100 "fnOrConstructor\|interface.*TsType"Repository: npmx-dev/npmx.dev
Length of output: 3442
🏁 Script executed:
cat -n shared/types/deno-doc.ts | sed -n '38,43p'Repository: npmx-dev/npmx.dev
Length of output: 262
🏁 Script executed:
cat -n shared/types/deno-doc.ts | sed -n '38,43p'Repository: npmx-dev/npmx.dev
Length of output: 262
🏁 Script executed:
cat -n server/utils/docs/format.ts | sed -n '116,122p'Repository: npmx-dev/npmx.dev
Length of output: 482
fnOrConstructor formatting drops generic constraints and constructor semantics.
Line 117 ignores the constraint field of type parameters (e.g. K extends keyof ...), and line 121 does not check the constructor boolean flag, so constructor signatures are rendered as plain function arrows instead of constructor syntax.
Proposed fix
function formatFnOrConstructorType(fn: NonNullable<TsType['fnOrConstructor']>): string {
- const typeParams = fn.typeParams?.map(t => t.name).join(', ')
+ const typeParams = fn.typeParams
+ ?.map(t => (t.constraint ? `${t.name} extends ${formatType(t.constraint)}` : t.name))
+ .join(', ')
const typeParamsStr = typeParams ? `<${typeParams}>` : ''
const params = fn.params.map(p => formatParam(p)).join(', ')
const ret = formatType(fn.tsType) || 'void'
- return `${typeParamsStr}(${params}) => ${ret}`
+ const ctorPrefix = fn.constructor ? 'new ' : ''
+ return `${ctorPrefix}${typeParamsStr}(${params}) => ${ret}`
}📝 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 formatFnOrConstructorType(fn: NonNullable<TsType['fnOrConstructor']>): string { | |
| const typeParams = fn.typeParams?.map(t => t.name).join(', ') | |
| const typeParamsStr = typeParams ? `<${typeParams}>` : '' | |
| const params = fn.params.map(p => formatParam(p)).join(', ') | |
| const ret = formatType(fn.tsType) || 'void' | |
| return `${typeParamsStr}(${params}) => ${ret}` | |
| } | |
| function formatFnOrConstructorType(fn: NonNullable<TsType['fnOrConstructor']>): string { | |
| const typeParams = fn.typeParams | |
| ?.map(t => (t.constraint ? `${t.name} extends ${formatType(t.constraint)}` : t.name)) | |
| .join(', ') | |
| const typeParamsStr = typeParams ? `<${typeParams}>` : '' | |
| const params = fn.params.map(p => formatParam(p)).join(', ') | |
| const ret = formatType(fn.tsType) || 'void' | |
| const ctorPrefix = fn.constructor ? 'new ' : '' | |
| return `${ctorPrefix}${typeParamsStr}(${params}) => ${ret}` | |
| } |
| const onProp = node.interfaceDef!.properties![0]! | ||
| const emitProp = node.interfaceDef!.properties![1]! |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the test file mentioned
find . -type f -name "format.spec.ts" | head -5Repository: npmx-dev/npmx.dev
Length of output: 105
🏁 Script executed:
# Get the file content to examine lines 20-21 and 42
cat -n test/unit/server/utils/docs/format.spec.ts | head -80Repository: npmx-dev/npmx.dev
Length of output: 3770
🏁 Script executed:
# Check the fixture file referenced in the test to understand array guarantees
find . -type f -name "linkdave@0.0.2-client.json" 2>/dev/null | head -3Repository: npmx-dev/npmx.dev
Length of output: 120
🏁 Script executed:
# Look for similar patterns of unchecked array access in the same directory
rg -n "properties!\[" test/unit/server/utils/docs/ -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 1024
🏁 Script executed:
# Examine the fixture file to see if the array has the required elements
cat test/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-client.json | jq '.interfaceDef.properties | length'Repository: npmx-dev/npmx.dev
Length of output: 62
🏁 Script executed:
# Also check the other fixture used in line 42
cat test/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-manager-events.json | jq '.interfaceDef.properties | length'Repository: npmx-dev/npmx.dev
Length of output: 62
🏁 Script executed:
# Look at the structure of the properties to verify they exist
cat test/fixtures/esm-sh/doc-nodes/linkdave@0.0.2-client.json | jq '.interfaceDef.properties[0:2]' | head -30Repository: npmx-dev/npmx.dev
Length of output: 718
🏁 Script executed:
# Check the TypeScript types for DenoDocNode to understand the property types
find . -type f -name "*.ts" -path "*/types/*" | xargs grep -l "DenoDocNode\|interfaceDef\|properties" | head -5Repository: npmx-dev/npmx.dev
Length of output: 87
🏁 Script executed:
# Read the DenoDocNode type definition to understand property types
cat -n ./shared/types/deno-doc.ts | head -100Repository: npmx-dev/npmx.dev
Length of output: 3094
🏁 Script executed:
# Search for interfaceDef definition specifically
rg -n "interfaceDef\|properties" ./shared/types/deno-doc.ts -A 3 -B 1 | head -50Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Continue searching for interfaceDef definition
cat -n ./shared/types/deno-doc.ts | tail -80Repository: npmx-dev/npmx.dev
Length of output: 2263
🏁 Script executed:
# Search more specifically for interfaceDef
rg -n "interfaceDef" ./shared/types/deno-doc.ts -A 5Repository: npmx-dev/npmx.dev
Length of output: 219
Ensure strict array bounds checking instead of relying on non-null assertions.
Lines 20–21 and 42 use non-null assertions (!) to bypass TypeScript's array bounds checking. Although the fixtures happen to contain the required properties, code should explicitly verify array access rather than asserting safety. Consider adding guards before indexed access to comply with type-safety guidelines.
it('event listener: interface properties with fnOrConstructor type (client.d.ts)', () => {
const node = loadFixture('linkdave@0.0.2-client.json')
- const onProp = node.interfaceDef!.properties![0]!
- const emitProp = node.interfaceDef!.properties![1]!
+ const properties = node.interfaceDef?.properties
+ if (!properties?.[0] || !properties[1]) {
+ throw new Error('Fixture is missing expected interface properties')
+ }
+ const onProp = properties[0]
+ const emitProp = properties[1]Line 42 should be similarly guarded before access.
📝 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.
| const onProp = node.interfaceDef!.properties![0]! | |
| const emitProp = node.interfaceDef!.properties![1]! | |
| it('event listener: interface properties with fnOrConstructor type (client.d.ts)', () => { | |
| const node = loadFixture('linkdave@0.0.2-client.json') | |
| const properties = node.interfaceDef?.properties | |
| if (!properties?.[0] || !properties[1]) { | |
| throw new Error('Fixture is missing expected interface properties') | |
| } | |
| const onProp = properties[0] | |
| const emitProp = properties[1] |
2 participants
🔗 Linked issue
resolves #1411
🧭 Context
formatType()only handled 4 type kinds (keyword,typeRef,array,union), but@deno/docproduces 21 kinds. When encountering unhandled kinds with an emptyrepr, the function returned"unknown".📚 Description
TYPE_FORMATTERSfor missing type kinds:fnOrConstructor,indexedAccess,typeOperator,typeLiteral,literal,thisTsTypeinterface with corresponding fieldsRecently,
deno_docis undergoing some changes; please see https://discord.com/channels/1464542801676206113/1464544758020968448/1476439287468654733. I'm not sure if these changes will cause any other problems.