-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
[code-infra] Upgrade react-docgen to v8 #47685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 37 commits
c15b02a
f558d76
d358ba0
d37263f
90e92a2
74efc23
371dc84
dc2367c
82bc7e1
0bb3470
3caac7d
66f2347
120bcae
1bb640d
dfc3689
d83fc71
b0dd5fa
b0cd87a
831c7f5
22076a6
004d90f
d67601c
9d15781
1bfc041
56371e3
0b2d1f4
28b3ecc
6d4a1d4
1825e0a
e3b9369
34845d2
0145c29
e3122f4
43c9fc8
73cf8a6
5bf034f
acd6909
e3c6346
e0d68da
525e94f
2bb4a9b
30071a9
9da3f78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,20 +34,52 @@ function getSymbolDocumentation({ | |
| }: { | ||
| symbol: ts.Symbol | undefined; | ||
| project: TypeScriptProject; | ||
| parentType?: ts.Type; | ||
| }): string | undefined { | ||
| if (symbol === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const decl = symbol.getDeclarations(); | ||
| if (decl) { | ||
| // @ts-ignore | ||
| const comments = ts.getJSDocCommentsAndTags(decl[0]) as readonly any[]; | ||
| if (comments && comments.length === 1) { | ||
| const commentNode = comments[0]; | ||
| if (ts.isJSDoc(commentNode)) { | ||
| return doctrine.unwrapComment(commentNode.getText()).trim(); | ||
| if (decl && decl.length > 0) { | ||
| // This behavior tries to replicate how TypeScript itself merges JSDoc comments | ||
| // It is a complex logic that changes based on the kind of declarations | ||
| // There is an open issue for it in: https://github.com/microsoft/TypeScript/issues/30901 | ||
| // | ||
| // For intersection types (A & B), the symbol may have multiple declarations. | ||
| // We need to handle three cases: | ||
| // 1. Intersection (type C = A & B): merge JSDoc from all declarations (deduplicated) | ||
| // 2. Interface extends (interface Z extends X, Y): use the (only) declaration's JSDoc | ||
| // 3. Interface override (interface W extends X { prop }): use the override's JSDoc (which is the only declaration) | ||
| // | ||
| // Note: TypeScript gives us: | ||
| // - Multiple declarations for intersection types (one from each constituent type) | ||
| // - Single declaration for interface extends (from the original interface) | ||
| // - Single declaration for interface override (from the overriding interface) | ||
|
|
||
| // Get JSDoc comments paired with their declarations | ||
| const declarationsWithComments = decl | ||
| .map((d) => { | ||
| const jsDocNodes = ts.getJSDocCommentsAndTags(d).filter((node) => ts.isJSDoc(node)); | ||
| const comment = | ||
| jsDocNodes.length > 0 | ||
| ? doctrine.unwrapComment(jsDocNodes[0].getText()).trim() | ||
| : undefined; | ||
| return { declaration: d, comment }; | ||
| }) | ||
| .filter((item) => item.comment !== undefined); | ||
|
|
||
| if (declarationsWithComments.length > 0) { | ||
| // If there's only one declaration with a comment, use it | ||
| // This handles both interface extends and interface override cases | ||
| if (declarationsWithComments.length === 1) { | ||
| return declarationsWithComments[0].comment; | ||
| } | ||
|
|
||
| // Multiple declarations with comments - this is the intersection case (type C = A & B) | ||
| // Merge JSDoc comments, deduplicating identical ones | ||
| const uniqueComments = [...new Set(declarationsWithComments.map((d) => d.comment))]; | ||
| return uniqueComments.join('\n'); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -116,7 +148,7 @@ function checkType({ | |
| return createObjectType({ jsDoc: undefined }); | ||
| } | ||
|
|
||
| const typeNode = type as any; | ||
| const typeNode = type; | ||
| const symbol = typeNode.aliasSymbol ? typeNode.aliasSymbol : typeNode.symbol; | ||
| const jsDoc = getSymbolDocumentation({ symbol, project }); | ||
|
|
||
|
|
@@ -396,16 +428,18 @@ function checkSymbol({ | |
| symbol, | ||
| location, | ||
| typeStack, | ||
| parentType, | ||
| }: { | ||
| project: PropTypesProject; | ||
| symbol: ts.Symbol; | ||
| location: ts.Node; | ||
| typeStack: readonly number[]; | ||
| parentType?: ts.Type; | ||
| }): PropTypeDefinition { | ||
| const declarations = symbol.getDeclarations(); | ||
| const declaration = declarations && declarations[0]; | ||
| const symbolFilenames = getSymbolFileNames(symbol); | ||
| const jsDoc = getSymbolDocumentation({ symbol, project }); | ||
| const jsDoc = getSymbolDocumentation({ symbol, project, parentType }); | ||
|
|
||
| // TypeChecker keeps the name for | ||
| // { a: React.ElementType, b: React.ReactElement | boolean } | ||
|
|
@@ -498,9 +532,9 @@ function squashPropTypeDefinitions({ | |
| }): PropTypeDefinition { | ||
| const distinctDefinitions = new Map<number, PropTypeDefinition>(); | ||
| propTypeDefinitions.forEach((definition) => { | ||
| if (!distinctDefinitions.has(definition.$$id)) { | ||
| distinctDefinitions.set(definition.$$id, definition); | ||
| } | ||
| // Always update so that the last definition's jsDoc wins | ||
| // This ensures that when types are intersected (A & B), the last type's documentation is used | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it went from first to last definition wins?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty much
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reasoning was more that usually we have
Ideally the best way is to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interface A extends B {}feels similar to B & Ato me, as in A's props should win over Bs, but that isn't how this code interpretes it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, A should win in the first, but B should in the second. 🫠 I checked it on the playground Inheritance picks the first time the prop appears 🙃 Intersection merges all declarations together.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ideally we take interface behavior, because that can't simply be altered to fix the comment. i.e. interface X extends Y, Z {
/** foo */
prop: number
}we can easily move For types, we can freely change the order, so it doesn't really matter what we do in terms of "first wins" or "last wins", we can pick what interfaces do and adjust order where needed. Instead of adding more
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've reworked it a bit to follow typescript, which required less changes in the end :) In a few cases it still made sense to add |
||
| distinctDefinitions.set(definition.$$id, definition); | ||
| }); | ||
|
|
||
| if (distinctDefinitions.size === 1 && !onlyUsedInSomeSignatures) { | ||
|
|
@@ -513,16 +547,20 @@ function squashPropTypeDefinitions({ | |
| types.push(createUndefinedType({ jsDoc: undefined })); | ||
| } | ||
|
|
||
| // Use the last definition's jsDoc so that when types are intersected (A & B), | ||
| // the last type's documentation wins | ||
| const lastDefinition = definitions[definitions.length - 1]; | ||
|
|
||
| return { | ||
| name: definitions[0].name, | ||
| jsDoc: definitions[0].jsDoc, | ||
| name: lastDefinition.name, | ||
| jsDoc: lastDefinition.jsDoc, | ||
| propType: createUnionType({ | ||
| // TODO: jsDoc from squashing is dropped | ||
| jsDoc: undefined, | ||
| types, | ||
| }), | ||
| filenames: new Set(definitions.flatMap((definition) => Array.from(definition.filenames))), | ||
| $$id: definitions[0].$$id, | ||
| $$id: lastDefinition.$$id, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -544,6 +582,7 @@ function generatePropTypesFromNode( | |
| project: params.project, | ||
| location: parsedComponent.location, | ||
| typeStack: [(componentType as any).id], | ||
| parentType: componentType, | ||
| }), | ||
| ); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that means if you change an interface to a type or vice-versa, the extracted comments suddenly can look different?
So these wouldn't produce the same output in the proptypes, right? There seems to be an open issue about this microsoft/TypeScript#30901. Your stance is to try emulate intellisense as closely as possible? I hate this behavior, but it also kind of makes sense to make sure the docs and the intellisense in vscode look the same. Maybe link to that open issue so we can periodically check if it ever gets fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, changing them could look different, which is the behaviour of the intellisense as you mentioned: https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgILIN4ChnIPQBUByMA9qcgXjsgA5Sm0BcyIArgLYBG0WAvliyhIsRCgBCmGoWJc4UStVz1GLdt14CsYAJ60UADWQBeNMgBkySZey4ZyOQC9FNFc1aceUfoOHR4SMgAwsgQAB6QIAAmAM5WADRmtvhEDnDOVK4M7upePtp6KACaJsGCCKQgMWDIYSxGpsnK2SwAjD4VVTU6LCWNNM2qyABM-EA
I find the internal behaviour of intellisense weird too, but I think it is sensible that we emulate what the users would actually see in the interface in order to better control it.
Personally I don't look at the libraries docs most of the time. I'm reading the types documentation in either the UI or the declarations, so this is technically a "bugfix" for me 😆
I'll mention the issue in code