Skip to content
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

[docs-infra] Allow JSDoc tags #42327

Merged
merged 2 commits into from
May 22, 2024

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented May 22, 2024

These changes allow JSDoc tags like @deprecated to be present in components and hook JSDoc comments without them being displayed in the docs. Before these changes, JSDoc tags would be displayed in the docs.

The benefits to allow JSDoc tags are:

  • Consumers get the info right in their IDE e.g. strikethrough styles and info in the popover when using a @deprecated API.
    Screenshot 2024-05-22 at 10 12 27
  • We'll be able to use JSDoc tags like @deprecated as the source of truth to display deprecation alerts in API pages. Further context #42280
  • We'll be able to use standard JSDoc syntax for things like links to doc pages.

@aarongarciah aarongarciah added the scope: code-infra Specific to the core-infra product label May 22, 2024
@mui-bot
Copy link

mui-bot commented May 22, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 9611931

@aarongarciah aarongarciah force-pushed the aarongarciah/docs-jsdoc-tags branch 4 times, most recently from fa4a14b to 4492bf7 Compare May 22, 2024 10:39
@aarongarciah aarongarciah marked this pull request as ready for review May 22, 2024 10:54
@aarongarciah aarongarciah added scope: docs-infra Specific to the docs-infra product and removed scope: code-infra Specific to the core-infra product labels May 22, 2024
@aarongarciah aarongarciah requested a review from a team May 22, 2024 11:06
@aarongarciah aarongarciah changed the title [code-infra] Allow JSDoc tags [docs-infra] Allow JSDoc tags May 22, 2024
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, you parse the JSDocs to do a distinction between the description and tag, and then render the enriched message and add the tags at the end

Looks clean ✅

Comment on lines +16 to +18
*
* @param id - The id of the MenuItem. If undefined, it will be generated with useId.
* @returns The stable ListContext value and the id of the MenuItem.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs API generation is using naming template to find the types to parse. That's why currently have wrong parameter table 🙈

image

const parameters = await extractInfoFromType(`${upperFirst(name)}Parameters`, project);
const returnValue = await extractInfoFromType(`${upperFirst(name)}ReturnValue`, project);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this, yeah! I was about to open an issue about it. It looks like some hooks like useMenuItemContextStabilizer and useOptionContextStabilizer are missing the *Parameters and *ReturnValue types. But even if they had those types, it looks like the API generation for the parameter table in hooks only works for hooks that accept a single object parameter. Same for the return value.

We also need to improve how we display the parameters table in the docs since it's not clear that the hook accepts a single object vs separated parameters when looking at it.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like the script generation should crash in those cases: https://next--material-ui.netlify.app/base-ui/react-menu/hooks-api/#use-menu-item-context-stabilizer. Forcing to either make the API private, or to use the right naming convention for its type.

@aarongarciah
Copy link
Member Author

aarongarciah commented May 22, 2024

From what I understand, you parse the JSDocs to do a distinction between the description and tag, and then render the enriched message and add the tags at the end

@alexfauquette exactly! Side note: doctrine is a dead project but we still use it on some docs infra stuff.

@aarongarciah aarongarciah force-pushed the aarongarciah/docs-jsdoc-tags branch from 4492bf7 to 9611931 Compare May 22, 2024 13:17
@aarongarciah aarongarciah merged commit c409a0c into mui:master May 22, 2024
22 checks passed
aarongarciah added a commit to aarongarciah/material-ui that referenced this pull request May 22, 2024
@@ -90,6 +90,8 @@ export interface HiddenProps {
* API:
*
* - [Hidden API](https://mui.com/material-ui/api/hidden/)
*
* @deprecated The Hidden component was deprecated in Material UI v5. To learn more, see [the Hidden section](/material-ui/migration/v5-component-changes/#hidden) of the migration docs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use absolute URLs here for links to work in IDE?

Copy link
Member Author

@aarongarciah aarongarciah May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should use absolute URLs for links inside comments, but this is an existing issue that needs to be tackled separately. We have this problem in all props e.g. https://github.com/mui/material-ui/blob/next/packages/mui-material/src/ListItem/ListItem.js#L372

JSDoc comments are used to generate the docs so we can't assume we're always in https://mui.com. See: https://github.com/mui/material-ui/blob/master/packages/api-docs-builder/ApiBuilders/ComponentApiBuilder.ts#L111-L117

We should probably use absolute URLs and adjust the docs infra to replace the base URL with the correct one. I'll open an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cherniavskii good catch!

@aarongarciah please assign me to the issue when you create it 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants