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] JSDoc links are broken #42361

Closed
aarongarciah opened this issue May 23, 2024 · 21 comments
Closed

[docs-infra] JSDoc links are broken #42361

aarongarciah opened this issue May 23, 2024 · 21 comments
Assignees
Labels
bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product

Comments

@aarongarciah
Copy link
Member

aarongarciah commented May 23, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/focused-bardeen-6cw9nz?file=%2Fsrc%2FApp.tsx

Steps:

  1. In the code pane of the CodeSandbox linked above, hover the disabled prop in <ListItem disabled />.
  2. In the info tooltip, click the ListItemButton link in the @deprecated message.
Screenshot 2024-05-23 at 17 59 52

Current behavior

The link points to "/material-ui/api/list-item-button/" i.e. the link is broken.

Expected behavior

The link should point to "https://mui.com/material-ui/api/list-item-button/".

Context

The docs infra uses JSDoc comments to generate documentation and relative URLs are used for links. The correct URLs are generated in the docs by appending the correct base URL but devs end up with broken links in their IDEs because of the relative URLs.

Your environment

No response

Search keywords: jsdoc, urls

@aarongarciah aarongarciah added status: waiting for maintainer These issues haven't been looked at yet by a maintainer scope: docs-infra Specific to the docs-infra product labels May 23, 2024
@aarongarciah aarongarciah changed the title JSDoc links are broken [docs-infra] JSDoc links are broken May 23, 2024
@DiegoAndai DiegoAndai added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 23, 2024
@tonygravell
Copy link
Contributor

@aarongarciah I'd like to take a look at this issue, but it looks like the codesandbox link is broken for me. Can you give more context on how to reproduce this issue?

@aarongarciah
Copy link
Member Author

@tonygravell sorry, the sandbox was private, you should be able to access it now. I've attached a screenshot to the description.

@tonygravell
Copy link
Contributor

Would it make sense to make them all use absolute URL's? It looks like some of the links in the file already have them.
Screenshot 2024-05-24 at 10 36 37 AM

@aarongarciah
Copy link
Member Author

aarongarciah commented May 24, 2024

Yes, the end goal is to use absolute URLs inside every JSDoc block so devs see the correct URLs in their IDE. But we need to do it in a way that doesn't break URLs that end up in the docs. In the following screenshot, the blue blocks are used to generate part of the docs:

  • The first blue block ends up on the API page.
    Screenshot 2024-05-24 at 16 51 24
  • The second blue block ends up in the props table in the API page.
    Screenshot 2024-05-24 at 16 51 57
  • The yellow block is inserted here automatically and AFAIK it's not used to generate any documentation, so it's safe to use absolute URLs there.

If we use absolute URLs in the blue blocks, anytime the docs are built under other base URLs (e.g. PR previews or https://next.mui.com/), they'll point to another domain, which is not what we want.

So the idea is to use absolute URLs in every JSDoc block and adapt the docs generation to replace the https://mui.com base URL with the correct base URL depending on the environment the docs are being built. This requires changes in at least:

Probably more things need to be adapted, so I'd love someone from @mui/docs-infra to confirm if this approach makes sense before we continue this effort.

@alexfauquette
Copy link
Member

Some JSDocs are already using the absolute URL

Having working links seems more important than supporting links in preview.

For the next.mui.com we could do a "replace all" with ](https://mui.com when starting the next branch. Ths fix the docs link but also make sure users are redirected to the up to date documentation if they are using the alpha/beta version

@aarongarciah
Copy link
Member Author

Thanks @alexfauquette! @tonygravell let me know if you're still interested in working on this and if you need any help.

@tonygravell
Copy link
Contributor

Okay, let me take a look and I'll reach out if I need be. Thanks!

@tonygravell
Copy link
Contributor

tonygravell commented May 30, 2024

For the next.mui.com we could do a "replace all" with ](https://mui.com when starting the next branch. Ths fix the docs link but also make sure users are redirected to the up to date documentation if they are using the alpha/beta version

@alexfauquette For the replace all, would I do this from the root? Are we trying to do this for all files or should I be adding a filter (*.d.ts, *.ts, *.md, etc.)?

@alexfauquette
Copy link
Member

It should only be about the JSDocs.

So I assume searching for packages/**/*.ts and packages/**/*.d.ts

By the way, most of those links are generated by those two lines of code:

https://github.com/mui/material-ui/blob/master/packages/api-docs-builder/ApiBuilders/HookApiBuilder.ts/#L112-L113
https://github.com/mui/material-ui/blob/master/packages/api-docs-builder/ApiBuilders/ComponentApiBuilder.ts/#L151-L154

It would be nice to either modify the constant, or better make it configurable, such that other user of the script can pick a different host when defining settings https://github.com/mui/material-ui/blob/master/packages/api-docs-builder-core/materialUi/projectSettings.ts

@tonygravell
Copy link
Contributor

tonygravell commented Jun 1, 2024

@alexfauquette I made a draft PR for now, I'm aware this doesn't fully solve the issue. Is this what you were thinking for your most recent comment about having a configurable host constant? I'm still not sure I understand the replace all part after looking at the code. This part still isn't clear to me: "For the next.mui.com we could do a "replace all" with ](https://mui.com" Could you try and explain that a little more?

@tonygravell
Copy link
Contributor

It might help me to understand the context. Is it that next.mui.com is an invalid URL/no longer useful and just needs to be replaced?

@alexfauquette
Copy link
Member

About the script modification

When a project is in alpha/beta phase, we have two documentation:

  • The docs at mui.com is the documentation of the last stable major. WHich code lives on master branch
  • The docs at next.mui.com is the documentation of the new major which is still in unstable mode, living on the next branch

So the code currently on master will be used to build v5 version of the lib and should direct users to the mui.com docs. And the one on next branch is used to build the v6.alpha which should redirect to next.mui.com

Your PR looks good. You will have to run pnpm proptypes and pnpm docs:api after modifying the host to see its impact.

It should solve most of the wrong links.

About the replace all

Some links are handmade. So we should also modify them by hand. For example this JSDocs comment is the same on master and next

* The [global variant](https://mui.com/joy-ui/main-features/global-variants/) to use.
* @default 'plain'

THis is more anecdotic than the links your PR fix

@tonygravell
Copy link
Contributor

@alexfauquette After adding those changes you recommended on my PR, I see a lot of changed files (160 total) in VSCode when running pnpm docs:api. It looks like its just updating the the URL. I'm assuming this is okay because its a relative path and if next.mui.com is correct, this should work now?
Screenshot 2024-06-04 at 4 12 09 PM

If that's true, this should be working.

Would I submit the changed files with my PR or is that for local dev only?

@alexfauquette
Copy link
Member

For me looks good, I run it to verify the modification. I added them to your PR since they look ok

I opened another PR (#42528) on top of yours to manually fix the handcrafted descriptions. @DiegoAndai are you ok with the two updates?

@tonygravell
Copy link
Contributor

@alexfauquette do you know what the argos failing check on my PR is about? Is that something I need to handle or is it optional? I haven't used that tool before.

@aarongarciah
Copy link
Member Author

@tonygravell I approved the changes in Argos. We have a few flaky tests at the moment. All green now.

@DiegoAndai
Copy link
Member

@DiegoAndai are you ok with the two updates?

@alexfauquette changes look good to me 👌🏼

@bharath391
Copy link

sooo good !
its clean...

@siriwatknp
Copy link
Member

@aarongarciah @DiegoAndai I think this issue can be closed, right?

@aarongarciah
Copy link
Member Author

@siriwatknp yes 👍

Copy link

github-actions bot commented Nov 8, 2024

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

No branches or pull requests

6 participants