[docs-infra] Simplify API sections typing

[alexfauquette]

Follow up of #43149

Previously

In the previous PR, The section props (PropertiesSection, ClassesSection, and SlotsSection) have been typed base on the @mui-internal/api-docs-builder types to enforce consistency.

In This PR

Section types

The sections now define the items they will render independently from the api-docs-builder with interfaces ClassDefinition, SlotDefinition, and PropertyDefinition. They come with processor that map slot, classes, props, and hooks params and return values to the appropriate definition

 <PropertiesSection
-    properties={parameters}
-    propertiesDescriptions={parametersDescriptions}
-    componentName={hookName}
-    hooksParameters
+    properties={hookApiProcessor({ // Or propsApiProcessor, or interfaceApiProcessor depending on the section to display.
+      kind: 'parameters',
+      hookName,
+      properties: parameters,
+      translations: parametersDescriptions,
+    })}
    level="h3"
    title="api-docs.parameters"
    titleHash={`${hookNameKebabCase}-parameters`}
/>

This migration is optional. See for example the typing of ClassesSectionProps. If the props are not the new ones, the processor will be applied by the ClassesSection. One other repository (I think only X is impacted) we will be able to remove those old options.

export type ClassesSectionProps = (
  | {
      classes: ClassDefinition[];
      componentClasses?: undefined;
      classDescriptions?: undefined;
      componentName?: undefined;
    }
  | {
      classes: undefined;
      componentClasses: ComponentClassDefinition[];
      classDescriptions: PropsTranslations['classDescriptions'];
      componentName: string;
    }
) & { ... }

Table of contents

The generation of the ToC can be done based in those ClassDefinition, SlotDefinition, and PropertyDefinition.

But because the MarkdownV2 which defines its ToC before rendering the Sections, I kept the old one and marked them as deprecated

Table of content entries are now typed

[mui-bot]

Netlify deploy preview

https://deploy-preview-43128--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2138bda

[alexfauquette]

If you take a close look, you will notice this line is removed in the tsx files. That's because the description property does not exist in the typing.

I will need to double-check with all components using this. But for example, base uses the @ignore on some props, and they never reach that place.

[alexfauquette]

Added to simplify the typing when using translator in helper functions

[Janpot]

Having a typing contract

top-notch. I think the strategy makes a lot of sense. add types => refactor => extract. Similar approach can be done between the pages and the output of the markdown generator. Or anywhere else we bridge between build time and runtime.

Make API Sections more independent from scripts results

I'd love for those {{...}} interpolations to some day be moved to MDX. In that sense, it would be helpful if we can already make them look more like rendering a component. Not necessarily in terms of JSX syntax, but in terms of how properties are defined

{{"component": "./SomeComponent", "prop1": "foo", "prop2": 123}}

Is spiritually equivalent to

import SomeComponent from "./SomeComponent"

// ...

<SomeComponent prop1='foo' prop2={123} />

[alexfauquette]

I'd love for those {{...}} interpolations to some day be moved to MDX. In that sense, it would be helpful if we can already make them look more like rendering a component. Not necessarily in terms of JSX syntax, but in terms of how properties are defined

@Janpot Not sure to understand your point on this one.

The API pages are not markdown rendered

The page file fetch the JSON and render the ApiPage component with the JSON data.

[Janpot]

The API pages are not markdown rendered

Ah yes ofcourse.

Long term we could perhaps think about making the constituents composable in markdown pages? e.g. due to complexity in the types we're at the moment writing some tables by hand in Toolpad that look a lot like these property tables.

[alexfauquette]

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

[alexfauquette]

This is the new way of defining API pages, such that if someone need to pass other kind of data, they can do it

The previous one should still work the time of the migration

[alexfauquette]

Generating the hash is now the responsibility of the processors.

[alexfauquette]

TODO on X side

[Janpot]

Two minor comments about naming and type-safety. The latter wasn't introduced in this PR, just something I noticed. They're just suggestions, no blockers.

[Janpot]

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.
For functions that transform one data type to another, I'd prefer to choose a name that includes both types. e.g. mapXtoY(x), xToY(x), yFromX(x). I like the latter as it mimics javascript API design (Object.fromEntries(), Array.from(), String.fromCharCode(), String.fromCodePoint()). I realize those aren't verbs neither, but the from/to clearly indicates a transformation.

[alexfauquette]

Effectively I was not super happy with those namings

About mapXtoY naming, my struggle is that the X is not easy to name since it's a mix of descriptions and translations. So I went with a getter naming get[Yyy]ApiDefinitions

[Janpot]

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 'undefined' without a warning.
If it's hard to guarantee type-wise and have this as a compile time error, we could add an invariant() call to turn this into a build time error instead of quietly starting to replace strings with 'undefined' at run time.

[alexfauquette]

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 conditions is defined depends on the presence of {{conditions}} in the description, which might be hard to define with types

[Janpot]

The way I would write this is to replace only within the check of whether the description contains {{conditions}}

// (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 invariant. But again, this is all polish. Doesn't have to be in scope for this PR.

[alexfauquette]

Thanks for the detailed explanation 👍

[alexfauquette]

Suggested change

	if (!conditions && description.search(/{{nodeName}}/) !== -1) {
	if (!nodeName && description.search(/{{nodeName}}/) !== -1) {

[Janpot]

clean