[docs-infra] Simplify API sections typing #43128
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { PropsTranslations, ComponentApiContent } from '@mui-internal/api-docs-builder'; | ||
| import kebabCase from 'lodash/kebabCase'; | ||
|
|
||
| export interface ClassDefinition { | ||
| className: string; | ||
| key: string; | ||
| hash: string; | ||
| description?: string; | ||
| isGlobal?: boolean; | ||
| isDeprecated?: boolean; | ||
| deprecationInfo?: string; | ||
| } | ||
|
|
||
| export interface ClassesApiProcessorParams { | ||
| componentClasses: ComponentApiContent['classes']; | ||
| classDescriptions: PropsTranslations['classDescriptions']; | ||
| componentName: string; | ||
| } | ||
|
|
||
| export function classesApiProcessor(params: ClassesApiProcessorParams): ClassDefinition[] { | ||
|
There was a problem hiding this comment. Not a blocker at all, just some reflections from reviewing this but I find the naming confusing. a "processor" is a noun, for a function I usually expect a verb. The meaning of the word "processor" is quite broad, it can mean anything. There was a problem hiding this comment. Effectively I was not super happy with those namings About |
||
| const { componentClasses, classDescriptions, componentName } = params; | ||
|
|
||
| return componentClasses.map((classDefinition) => { | ||
| return { | ||
| ...classDefinition, | ||
| description: | ||
| classDescriptions[classDefinition.key]?.description | ||
| ?.replace(/{{conditions}}/, classDescriptions[classDefinition.key].conditions!) | ||
|
There was a problem hiding this comment. Is there any way to get rid of these non-null assertion operators? It removes type safety and an upstream bug can be introduced that essentially turns this replacement into There was a problem hiding this comment. I went with the following solution if (!conditions && description.search(/{{conditions}}/) !== -1) {
throw Error(
`In ${componentName} the class "${classDefinition.className}" description with "{{conditions}}" but without \`conditions\` to replace it.`,
);
}It adds the runtime error, but does not fix the typing, because the fact that There was a problem hiding this comment. The way I would write this is to replace only within the check of whether the description contains // (use .includes())
if (!conditions && description.includes(/{{conditions}}/)) {
throw new Error('...')
}
// This is suboptimal, a type checker can't relate the previous condition with the following replacement
return {
// we're replacing, even when we know there is nothing to replace
description: description.replace(/{{conditions}}/, conditions!),
}The solution requires a refactor // make description a mutable variable
if (description.includes(/{{conditions}}/)) {
// Use an invariant to assert the condition, this would be a programming error
// (don't use invariant when an error comes from user input)
invariant(conditions, 'conditions should be defined if description includes {{conditions}}')
// only replace when it makes sense
descripton = description.replace(/{{conditions}}/, conditions)
}
return {
description
}It's cleaner and more defensive imo. non-null assertion is always a code smell. I have never seen a use-case that isn't better solved with a refactor or an There was a problem hiding this comment. Thanks for the detailed explanation 👍 |
||
| ?.replace(/{{nodeName}}/, classDescriptions[classDefinition.key].nodeName!) ?? | ||
| classDefinition.description, | ||
| deprecationInfo: classDescriptions[classDefinition.key]?.deprecationInfo, | ||
| hash: `${kebabCase(componentName)}-classes-${classDefinition.className}`, | ||
| }; | ||
| }); | ||
| } | ||
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.
Need to bring that feature back. I initially removed it because I remembered that with the table layout it was useless, but did not manage to find back this conclusion on github