-
Notifications
You must be signed in to change notification settings - Fork 80
Improve handling of docs from third-party libraries #275
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
base: master
Are you sure you want to change the base?
Conversation
Symbols with no declarations| // Some symbols e.g. JSX.LibraryManagedAttributes have no declarations | ||
| if (!name.declarations || name.declarations.length === 0) { | ||
| continue; | ||
| } |
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.
I think this comes from React, declarations was undefined.
sphinx_js/js/convertTopLevel.ts
Outdated
| return content | ||
| .map((x) => { | ||
| if (x.kind === "code") { | ||
| return { type: "code", code: x.text }; | ||
| } | ||
| if (x.kind === "text") { | ||
| return { type: "text", text: x.text }; | ||
| } | ||
| console.warn(`Not implemented:`, x); | ||
| return null; | ||
| }) | ||
| .filter(Boolean) as DescriptionItem[]; |
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.
I encountered this due to our use of the events library. The CommentDisplayPart was the following:
{
kind: 'inline-tag',
tag: '@link',
text: 'defaultMaxListeners',
target: DeclarationReflection {
id: 70,
name: 'defaultMaxListeners',
kind: 1024,
flags: ReflectionFlags { flags: 1048 },
comment: undefined,
children: undefined,
documents: undefined,
childrenIncludingDocuments: undefined,
groups: undefined,
categories: undefined,
variant: 'declaration',
sources: [ [SourceReference] ],
escapedName: 'defaultMaxListeners',
type: IntrinsicType { name: 'number', type: 'intrinsic' },
typeParameters: undefined,
signatures: undefined,
indexSignatures: undefined,
getSignature: undefined,
setSignature: undefined,
defaultValue: undefined,
overwrites: undefined,
inheritedFrom: ReferenceType {
type: 'reference',
name: 'Chooser.defaultMaxListeners',
typeArguments: undefined,
highlightedProperties: undefined,
qualifiedName: 'Chooser.defaultMaxListeners',
package: undefined,
externalUrl: undefined,
refersToTypeParameter: false,
preferValues: false,
_target: 898
},
implementationOf: undefined,
extendedTypes: undefined,
extendedBy: undefined,
implementedTypes: undefined,
implementedBy: undefined,
typeHierarchy: undefined,
readme: undefined,
packageVersion: undefined
}
}I'm not sure whether failing loudly and stopping the build was intentional. If it were, I suppose we can look for alternatives. Maybe handling inline-tag?
I'm pretty sure Wagtail should've used EventTarget rather than events' EventEmitter, so we could also refactor our code instead...
sphinx_js/js/convertTopLevel.ts
Outdated
| console.warn(`Not implemented:`, x); | ||
| return null; |
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.
We could do something like this:
return content.map((x): DescriptionItem => {
switch(x.kind) {
case "code":
return { type: "code", code: x.text };
case "text":
return { type: "text", text: x.text };
case "inline-tag":
return { type: "text", text: x.text };
case "relative-link":
return { type: "text", text: x.text };
}
});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.
This is a bit lazy, it would be better to add inline-tag and relative-link cases to DescriptionItem, but we could put a TODO here.
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.
| default: | ||
| throw new Error("Not implemented"); |
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.
Let's not include this default case because then the typescript typechecker will give a type error if there are cases that we haven't covered.
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.
Without the default case, TypeScript requires the return value to be Array<DescriptionItem | undefined>. Is that what we want?
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.
For me it works fine. Did you run npm i?
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.
See #276:
https://github.com/pyodide/sphinx-js/actions/runs/16121106961/job/45486860109#step:6:31
It should typecheck fine if your development environment is set up correctly.
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.
Ah, sorry about that, thanks! It does work after that, but now it detects relative-link being incorrect because it was added in typedoc 0.26, while our package.json uses typedoc ^0.25.13.
Would you rather drop relative-link, or update typedoc (or maybe add a version check...)? (Also, it seems package-lock.json needs updating...)
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.
We can skip the typecheck job against typedoc 0.25 and only run it on the 0.27 and 0.28 jobs.
Hey, thanks for releasing a new version of the package!
I tried to use it to build Wagtail's docs, but unfortunately encountered a few errors that were raised during the build process due to third-party libraries that we use.
Here are a couple fixes I added to get the build working. Not sure how to write the tests though. Let me know if you'd like any changes!