Refactor/simplify extraction.ts#8898
Conversation
16024a4 to
952d871
Compare
The goal of this commit is to make `extraction.ts` easier to follow. Since we are working with the TypeScript AST, we have to accept that the code will be somewhat dense, and we will need to ask maintainers to do some amount of studying to develop a mental model in their head. However the more we can do to assist this process the better. To this end, this commit: - Adds comments to illustrate steps in the parsing process and underlying nodes in the AST that are being visited - Renames some functions to more accurately describe their contents - Removes unnecessary `istanbul ignore` comments in favor of either adding tests that exercise the underlying logic or using assumptions (non-null assertions). - Rewrite some JSDoc so it's less technical and more natural sounding
952d871 to
57ad0bd
Compare
mcmire
left a comment
There was a problem hiding this comment.
Hey @cryptodev-2s, I hope you don't mind that I made some changes to extraction.ts as I went through it and tried to understand it. Hopefully these make sense but let me know if you have any questions.
| deprecatedLines.push( | ||
| comment ? `**Deprecated:** ${comment}` : '**Deprecated:**', | ||
| ); | ||
| } else if (tagName === 'param' && NodeGuards.isJSDocParameterTag(tag)) { | ||
| const nameNode = tag.getNameNode(); | ||
| // istanbul ignore next: `@param` tags without a name aren't valid JSDoc. | ||
| if (!nameNode) { | ||
| const paramName = nameNode.getText(); |
There was a problem hiding this comment.
It's true that if a @param tag does not have a name, it will be flagged by the ESLint JSDoc lint rules. However, ts-morph does not care about that. We instead explicitly check for this kind of tag and ignore it.
| if (NodeGuards.isTypeQuery(typeArg)) { | ||
| const type = typeArg.getType(); | ||
| if (type.isStringLiteral()) { | ||
| return type.getLiteralValueOrThrow() as string; |
There was a problem hiding this comment.
We don't need to have various strategies of resolving the value of a string literal. The getLiteralValueOrThrow method on the type does this already. So we can simplify this.
| */ | ||
| function collectMessengerCapabilityTypeDeclarations( | ||
| function findMessengerTypeAliases( |
There was a problem hiding this comment.
I split this function off from collectMessengerCapabilityTypeDeclarations because it seemed like a logical step in the process that deserved its own name.
| * @returns The list of type aliases that represent messenger capabilities among | ||
| * the given messenger types. | ||
| */ | ||
| function findAllMessengerCapabilityTypeDeclarations( |
There was a problem hiding this comment.
I renamed this from collectMessengerCapabilityTypeDeclarations to findAllMessengerCapabilityTypeDeclarations. This was mainly to match recursivelyFindMessengerCapabilityTypeDeclarations.
| */ | ||
| function walkCapabilityTypes( | ||
| function recursivelyFindMessengerCapabilityTypeDeclarations( |
There was a problem hiding this comment.
I renamed walkCapabilityTypes to recursivelyFindMessengerCapabilityTypeDeclarations as I felt like "walk" didn't accurately convey what we are trying to do. We are not merely visiting nodes, we are also capturing the capability types.
| if (resolvedType.isStringLiteral()) { | ||
| // Type assertion: There aren't any type guards we can use to narrow this | ||
| // type further. | ||
| const literalValue = resolvedType.getLiteralValueOrThrow() as string; |
There was a problem hiding this comment.
As elsewhere, we don't need to use multiple strategies to resolve the string literal, we can just ask the type checker to resolve it for us.
| if ( | ||
| // istanbul ignore next: capability properties always have identifier names. |
There was a problem hiding this comment.
We don't need to make this assumption, we can check for it and exercise this logic in a test.
| return null; | ||
| } | ||
|
|
||
| // A `type` property without an explicit type wouldn't compile. |
There was a problem hiding this comment.
Changed the istanbul ignore to a non-null assertion to better highlight that we are making an assumption.
| // We already determined in | ||
| // `recursivelyFindMessengerCapabilityTypeDeclarations` that the declaration | ||
| // is a type reference. | ||
| // TODO: Capture the following information ahead of time so we don't need to |
There was a problem hiding this comment.
There are more refactors we can do to simplify this function, but I decided to stop short of that so that this PR didn't blow up.
| */ | ||
| export type ExtractedMessengerCapabilityType = { | ||
| export type MessengerCapabilityPacket = { |
There was a problem hiding this comment.
While it's true the information captured here does come from a capability type, I felt that saying that it is a type was confusing. Plus, we are using "item" elsewhere, so it didn't feel like we had a good term we could use consistently for this concept. I came up with "packet" as I felt like it was a good word to represent the bundle of information we've collected for a capability.
Note that I didn't update the use of "item" across the package. I only updated this type. I will make more changes in another PR to address that inconsistency.
0862ed5
into
feat/messenger-docs-site
Explanation
The goal of this commit is to make
extraction.tseasier to follow. Since we are working with the TypeScript AST, we have to accept that the code will be somewhat dense, and we will need to ask maintainers to do some amount of studying to develop a mental model in their head. However, the more we can do to assist this process, the better.To this end, this commit:
istanbul ignorecomments in favor of either adding tests that exercise the underlying logic or using assumptions (non-null assertions).References
Checklist
Note
Medium Risk
Changes core AST extraction logic that drives published API docs; behavior should stay equivalent but edge-case handling shifted to type-checker resolution and explicit skips.
Overview
Refactors
platform-api-docsmessenger extraction for readability and safer edge-case handling, without changing the public doc-generation pipeline’s overall shape.extraction.tsis reorganized into clearer phases: find*Messengertype aliases, recursively walk Actions/Events type parameters to collect capability declarations, then extractMessengerCapabilityPacketrecords (renamed fromExtractedMessengerCapabilityType) via inline object literals orControllerGetStateAction/ControllerStateChangeEventconstructors. Discovery and extraction helpers are renamed and documented with AST-oriented comments; JSDoc handling is tightened (e.g. skip nameless@paramtags, bare@deprecated).Type resolution for capability
typestrings and constructor namespace args now leans on the type checker (getType().isStringLiteral()) instead of dedicated template-literal /typeofhelpers. Invalid shapes are skipped explicitly (missingtype/handler/payload, numerictype, qualifiedNs.Typereferences, non–string-literal constructor args, method signatures mixed into object types).Tests in
extraction.test.tsadd broad coverage for those skip/fallback paths; severalistanbul ignorebranches are removed in favor of tests or compile-time assumptions.generate.ts,markdown.ts, andtypes.tsonly adopt the new type name.Reviewed by Cursor Bugbot for commit 57ad0bd. Bugbot is set up for automated code reviews on this repo. Configure here.