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

Upgrade to Docusaurus 3 #1335

Draft
wants to merge 15 commits into
base: staging
Choose a base branch
from

Conversation

fill-the-fill
Copy link
Collaborator

@fill-the-fill fill-the-fill commented Oct 7, 2024

Checklist

Updating documentation or Bugfix

Hey 😎

I finally got some time to play around and update Docusaurus 3+.

I've made quite a few changes, and everything seems to be working now, including the previously broken Changelog.

The main issue was with pulling markdown files such as CIPS and others. Some of the files didn’t follow the correct formatting, which caused breaks on our side. Creating a special case for each file would have been a nightmare since every file that was pulled had its own missing comma or bracket that was really hard to track.

Since the biggest issue with upgrading to Docusaurus 3.0 was the transition to MDX, one of the biggest game-changers that helped was Experimental Commonmark. That quickly took care of the issue.

I added the following code to docusaurus.config.js:

  markdown: {
    format: "detect"
  },

However, this ended up breaking some of our markdown files, specifically those using the <Tabs> element. The solution I came up with was to transition those files from .md format to .mdx. I've decided to leave the decision of transitioning all the files to .mdx up to everyone else.

Please feel free to test it out locally and check if I've missed anything, thank you!

@rphair rphair added the enhancement New feature or request label Oct 7, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@fill-the-fill - this is very welcome and I would be inclined to merge it rather than remain on the obsolescent version of Docusaurus (FYI for group: this builds v3.5.2).

It builds a working site locally that I can't find any UI/content problems with, but the diagnostic warnings generated by the yarn build are huge... enough to overwhelm any messages about legitimate problems or things to clean up. This includes reported broken links and missing anchors on pages that appear to be working correctly on the built site.

The main issue was with pulling markdown files such as CIPS and others. Some of the files didn’t follow the correct formatting, which caused breaks on our side.

I don't know what to make of this, since I can personally guarantee the CIP editors have been rigorous about following Markdown formatting conventions, and have been fixing linking & other formatting issues that caused things to break on the Dev Portal for the 2+ years I've been involved in both projects. Any real breach of Markdown rules would have been identified on the previous version of Docusaurus.

It seems likely that CIP formatting problems would be in how the files are translated after downloading from GitHub... e.g. there are references to these "broken anchors" on many CIPs, for anchors that don't exist in the source material:

- Broken anchor on source page path = /docs/governance/cardano-improvement-proposals/CIP-0014:
   -> linking to CIP-0001#cip-format-and-structure (resolved as: /docs/governance/cardano-improvement-proposals/CIP-0001#cip-format-and-structure)
   -> linking to CIP-0001#cip-workflow (resolved as: /docs/governance/cardano-improvement-proposals/CIP-0001#cip-workflow)

Likewise there are reported problems on the documentation pages, in some cases broken links to autogenerated content like the section headings in the article sidebar.... though these were working normally for the pages I checked.

So I can approve this if this is the best we can get for now, but I do think it would first be worth a look at what generates this huge trail of debugging output & solve those cases one by one before merging this. If any source material on the CIPs repository is an issue then let's identify such problems here and I'll fix them upstream immediately.

@fill-the-fill
Copy link
Collaborator Author

fill-the-fill commented Oct 7, 2024

@rphair

I don't know what to make of this, since I can personally guarantee the CIP editors have been rigorous about following Markdown formatting conventions, and have been fixing linking & other formatting issues that caused things to break on the Dev Portal for the 2+ years I've been involved in both projects. Any real breach of Markdown rules would have been identified on the previous version of Docusaurus.

It seems likely that CIP formatting problems would be in how the files are translated after downloading from GitHub... e.g. there are references to these "broken anchors" on many CIPs, for anchors that don't exist in the source material:

Regarding that, I've ran npx docusaurus-mdx-checker (suggestion from here) and ran into some errors with CIP files specifically, instead of changing them one by one or opening a prs to CIP repo, I figured this would be the best solution at the moment.

It's not really the problem with how it's documented, it's how Docusaurus wants it to be :D

@katomm
Copy link
Member

katomm commented Oct 7, 2024

I don't get why you cut out all the truncate comments in the blog posts, we need them for the preview.

Also the md to mdx seems to be not necessary, what's the reasoning behind that? Thanks.

@katomm
Copy link
Member

katomm commented Oct 7, 2024

This PR contains too many problems, assumptions and decisions that will cause us problems in the near future. In this state it should be converted in a draft.

@katomm katomm marked this pull request as draft October 7, 2024 13:58
@fill-the-fill
Copy link
Collaborator Author

fill-the-fill commented Oct 7, 2024

I don't get why you cut out all the truncate comments in the blog posts, we need them for the preview.

Also the md to mdx seems to be not necessary, what's the reasoning behind that? Thanks.

I've stated above the reasoning behind converting certain files from .md to .mdx, you are more than welcome to test out those files in .md format and see it for yourself.

blog .md:

Screenshot 2024-10-07 at 17 35 05

blog .mdx:

Screenshot 2024-10-07 at 17 35 47

@fill-the-fill
Copy link
Collaborator Author

I've added back truncates and fixed most of the warnings. At the moment the only warnings are CIP files related.

@rphair
Copy link
Collaborator

rphair commented Oct 7, 2024

At the moment the only warnings are CIP files related.

@fill-the-fill it's true: the broken rendering of the CIP content from GitHub still generates a lot of spurious warnings. Are you suggesting that the Markdown content of the CIPs themselves is the true reason for this? If so, can you give an example in the actual CIP content?

@fill-the-fill
Copy link
Collaborator Author

fill-the-fill commented Oct 8, 2024

@rphair

For example, in CIP-0001, line 338 in sentence:

Once a proposal has reached all requirements for its target status (as explained in [Statuses](#Statuses)) and has been sufficiently and faithfully discussed by community members, it is merged with its target status.

Example: [Statuses](#Statuses)) should be [Statuses](#statuses))

Those warnings won't break the website, its mostly just redirecting to a specific section on the same CIP page. In our case it just won't jump to that section when being clicked. Besides that, all other warnings should be cleared out now.

You can take at the latest commit build in order to verify that the warnings are removed.

I've managed to remove most of the warnings this morning by adjusting CIP and CPS scripts, but here inside of build is the list of warnings that should probably be taken care of on CIP repo instead

@rphair
Copy link
Collaborator

rphair commented Oct 8, 2024

thanks @fill-the-fill - I'd be ready to post an issue on the CIPs repo to decide on a standard for anchor links that will be most portable outside of GitHub, including Docusaurus. But first I need to understand exactly what this version of Docusaurus does not "like" about these links.

I thought it might be a matter of capitalisation (with all lowercase being canonical), but while that explains the first warning (CIP-0001) it's not an issue with the second (CIP-0002 first item): since the anchor Docusaurus "likes" is also used everywhere in the CIP-0002 source.

And at the end you see Docusaurus selecting "canonical" links that have initial or camel capitalisation, so it can't be as simple as that (see here, where GitHub generates a lowercase link while Docusaurus says it wants initial capitalisation).

So unless you can explain the rules that links in the source documents are expected to follow, we may need more verbose information about how the warnings are being flagged... we'd need a line number and/or other precise context, not just the file that link is in. (Could you set an option in the build script that's more verbose about where the warning is?)

Once we can put those rules into an issue on the CIPs repository then we might start making these corrections preemptively. Authors might not be able to write their documents that way, but we've talked about adding a GitHub CI for the CIPs repo that will do some format massage & file updates when checking in pull requests & we could add something there when that is set up.

@fill-the-fill
Copy link
Collaborator Author

@rphair thanks for your reply. It seems like the issue with the example in CIP-2 is that some of the links are broken and can't be read, look at the example bellow:

gif

Here is another issue, those should be redirecting to a specific part of the page, but they're not recognized, in the example bellow you can see I'm clicking on the link, usually it goes to that part of the page, in this example nothing happens:

gif2

@fill-the-fill
Copy link
Collaborator Author

fill-the-fill commented Oct 8, 2024

So unless you can explain the rules that links in the source documents are expected to follow, we may need more verbose information about how the warnings are being flagged... we'd need a line number and/or other precise context, not just the file that link is in. (Could you set an option in the build script that's more verbose about where the warning is?)

In my opinion, this is not needed, but I could be wrong, for example:

-> linking to #accumulated-coin-selection (resolved as: /docs/governance/cardano-improvement-proposals/CIP-0002#accumulated-coin-selection)

Easily can be found by searching #accumulated-coin-selection. I don't believe there are any duplicates in the warnings.

Besides that, it shouldn't be a blocker to proceed with this PR, imho

@rphair
Copy link
Collaborator

rphair commented Oct 8, 2024

Easily can be found by searching #accumulated-coin-selection. I don't believe there are any duplicates in the warnings.

There are aren't duplicate warnings, but the links which generate the warnings are duplicated in individual CIP documents. See #requested-output-set in CIP-0002 (as quoted in #1335 (comment)) for a good example: this anchor appears 20 times in the document, so without a line number you'd have to investigate all of them to find which was generating the warning. 😬

@fill-the-fill
Copy link
Collaborator Author

Easily can be found by searching #accumulated-coin-selection. I don't believe there are any duplicates in the warnings.

There are aren't duplicate warnings, but the links which generate the warnings are duplicated in individual CIP documents. See #requested-output-set in CIP-0002 (as quoted in #1335 (comment)) for a good example: this anchor appears 20 times in the document, so without a line number you'd have to investigate all of them to find which was generating the warning. 😬

I will come back to you very soon

@fill-the-fill
Copy link
Collaborator Author

fill-the-fill commented Oct 8, 2024

@rphair

  • CIP-1 :
  • CIP-2 :
    • This file have big amount of broken links, I believe it should be tested from top to bottom.
  • CIP-26:
    • Line 483: (#special-case--phase--1-monetary-policies)
  • CIP-71:
    • Line 79: (#Creation-of-Ballot-Without-Casting-a-Vote)
    • Line 103: (#Creation-of-Ballot-Without-Casting-a-Vote)
    • Line 142: (#Ballot-Box---Smart-Contract)
    • Line 209: (#Ballot-Box---Smart-Contract)
    • Line 209: (#Motivation)
    • Line 219: (#Ballot---Plutus-Minting-Policy)
    • Line 223: (#Ballot---Plutus-Minting-Policy)
    • Line 223: (#Ballot-Counter---Authorized-Wallet)
  • CIP-86
    • Line 767: (#rationale)
  • CIP-88
    • Line 178: (#6--cip-specific-information)
  • CIP-95
    • Line 44: (#acknowledgements)
  • CIP-108
    • Line 30: (#acknowledgements)
  • CIP-119
    • Line 27: (#acknowledgements)
    • Line 227: (#paymentAddress)

@rphair
Copy link
Collaborator

rphair commented Oct 8, 2024

thanks @fill-the-fill - that's very helpful. I do think eventually we should have diagnostics from yarn build itself about where the exact problems are, but for these I'll address them upstream after finishing some work deadlines this week or two (referring back here & marking up the outline above) and then file an issue if these problems have common elements, so that other editors & maybe authors can be aware of them (cc @Ryun1 @perturbing).

@fill-the-fill
Copy link
Collaborator Author

@rphair sounds good, let me know if there is anything else I can do

@fill-the-fill
Copy link
Collaborator Author

Ok looks like build broke again, I will check it soon

@katomm
Copy link
Member

katomm commented Oct 17, 2024

Thanks for the effort again @fill-the-fill and everyone that is participating here. What I'm concerned about is: the list of compromises is getting longer and longer to maintain this. We are at a point where it is questionable not to remove the CIP from the portal. cips.cardano.org has been greatly improved in the last few months, you could try to index that so that you can still search for CIP content with the Developer Portal search but then land directly on cips.cardano.org.

@rphair
Copy link
Collaborator

rphair commented Oct 17, 2024

@katomm that's a compelling suggestion, but I would want to bring in the last known devs of cips.cardano.org (@ptrdsh @aldo555, cc @KtorZ) because the content there has been out of sync with the CIP GitHub material for long periods:

and is currently out of sync again (as seen in this last enormous & high visibility update to CIP-0001):

as well as this very high visibility addition to the CIP archive, merged yesterday, still not being synced:

Last time, as reported in the PR, the issue was an "expired token" but since less than 1 month had gone by when we observed this was out of date I don't think it could be a token problem again (?). So I would be OK with coupling the Dev Portal to the CIPs web site if both:

  1. CIP editors (cc @Ryun1 @perturbing @Crypto2099) and Dev Portal contributors could be informed about exactly how cips.cardano.org gets built;
  2. that information came with an assurance & some points of contact how to monitor & report consistency problems.

@aldo555
Copy link

aldo555 commented Oct 17, 2024

@rphair We will very soon be updating cips.cardano.org to be automatically rebuilt once a push is made to the cips repo.

@katomm
Copy link
Member

katomm commented Oct 17, 2024

Unfortunately a bit offtopic but helps me to substantiate my argument above: build breaks now because of CIP-127
photo_2024-10-17_21-40-32

@rphair
Copy link
Collaborator

rphair commented Oct 18, 2024

no @katomm that's a welcome comment & not really off topic because it was the basis for just referring the CIP other editors again back here in a parallel thread. Here's the other one of the 2 with the same potential breakage (at least in GitHub's YAML parser, a backtick is only a problem in the initial position: but its use will be imitated by the community if we allow it anywhere):

10:matangi$ grep ^Title: */README.md | grep \`
CIP-0101/README.md:Title: Integration of `keccak256` into Plutus
CIP-0127/README.md:Title: `ripemd-160` hashing in Plutus Core

I'll push through a PR to fix these upstream right away & have asked the others for opinions about #1335 (comment).

@rphair
Copy link
Collaborator

rphair commented Oct 18, 2024

@rphair
Copy link
Collaborator

rphair commented Oct 18, 2024

Builds should no longer be failing for this reason, as of cardano-foundation/CIPs@e4c5f48.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants