Docusaurus v2 Beta 17 Upgrade

[Mercurial]

This PR upgrades the cardano-developer-portal to the latest https://github.com/facebook/docusaurus stable beta.

Notable Changes

• colorMode config is now depreciated, customization now requires swizzling the theme although this is a safe and recommended thing to do if really needed.

• docusaurus build seems to check git history for the files its compiling, that means auto-generated files now need to be committed to git and just be updated when needed. Added a separate yarn scrap to scrap CIP Token Registry and Serialization Lib docs

Everything else seems to be fully compatible with the upgrade.

Let me know if you guys have any questions

[katomm]

Thanks, Clark. I appreciate you being active here again. :)) What exactly has been changed in beta 17 that makes it necessary to version track auto-generated files? Could you please point me to that change?

This was not done for a good reason as it creates a bunch of bloat and is completely unnecessary.

[Mercurial]

Thanks, Clark. I appreciate you being active here again. :)) What exactly has been changed in beta 17 that makes it necessary to version track auto-generated files? Could you please point me to that change?

This was not done for a good reason as it creates a bunch of bloat and is completely unnecessary.

Hi Tommy,

Great question, and its actually very simple to explain.

When you try to run yarn build with files that is not being tracked by git docusaurus will simply produce an error as shown in the screenshot below:

Whereas you don't see this in previous versions of docusaurus. In terms of the tradeoffs caused by this upgrade I do agree with your perspective but again it's just a certain perspective.

One could also have a perspective that all files that is deployed live should be a version that matches what is committed in the repo. Instead of having the deployment have a variation of the committed files depending on when the scraping function was run (presumably during CI/CD production deploy).

As for describing the generated files as bloat, its again a perspective for me, since in-order to replicate developer portal in your local machine you would need these files anyway, hence whether they are included in the repo or not. People contributing / developing it will still encounter and see these files, it's just an illusion that it's not seen in the repo. So I respectfully disagree that they are bloat. To me they are simply documents that are equally as important as the handwritten ones.

On the other side of the argument, the git history issue could simply go away in beta-18 but that's not a guarantee and if not the Cardano Foundation Developer Portal has 2 choices, whether they will be stuck with beta-14 forever or adapt to the change and keep it updating to latest docusaurus.

As the main person that was looking into the technicalities of Docusaurus at the time, it is my opinion that it is important for the Developer Portal to be constantly updated with the Docusaurus latest "stable" beta version to take advantage of the improvements / fixes / advancements that has been done between version updates and ensure future compatibility.

We definitely don't want a situation down the road where Cardano Dev Portal is still stuck at beta-14 meanwhile Facebook releases not beta version of v2 or even v3 where it could be much more harder to be compatible (in my mind is more like gradual compatibility, new features is just a bonus)

In terms of what's new in beta-17 to make it a worth while upgrade, I'm not sure if there are anyway besides fixes / improvements. As I've said the main motivation for upgrading is to maintain gradual compatibility.

Cheers,
Clark

[katomm]

I suggest we first look at exactly what is causing this problem before drawing any conclusions. Also unclear this "CIP-New.md" is not a file that should be created. Without looking at it closely, the probability is high that something else is creating this problem here.

The best would be to create a Docusaurus upgrade in a clean separate PR not bundled with many other changes.

[Mercurial]

That file was an example actually, and before the example I replicated this error with just exactly two steps: a docursaurus upgrade and then a yarn build as you have described which the lead to this PR.

This PR is basically an packages upgrade change and then it broke yarn build so the next step was to fix the issue then submit the PR.

So I am confident that docusaurus upgrade simply won't accept random files that's not in repository other wise it produces this error.

So no further steps are required, but you are free to test the same thing on your side.

The example CIP was my example because I've already fixed the issue for the real CIP files but before that they were also broken right after the docusaurus upgrade.

In other words I've already done the work to check it and know the cause, then fixed it and that's why this PR exists now. 😄

[katomm]

Thanks, Clark👍, I looked into it and can confirm the issues and changes. I'm still hesitant on the gitignore file changes and I would like to clarify something:

The word "bloat" should not judge the importance of these files. Of course, they are important. At the moment we were excluding this in gitignore because I was a little afraid that many people then always post with ShowCase PR etc dozens of automatically-generated files. (which indeed "bloat" the PR readability)

It becomes even more difficult for the reviewers to see if the changes are automatic or if people have written manually in the files.

In addition, it is no longer clear to people what is being generated automatically and they may make changes to the generated files.

All this together with the uncertainty of whether the problem exists only in v17, is enough to discuss this first and not to change the gitignore right now.

Most important: why would Docusaurus change this and not mention it in the release notes? (That alone is still an indication that this is simply a bug) And my suggestion is not to rush things now because of this version.

Happy to hear other opinions on whether we want to version track auto-generated files (and we will have many more) and how we want to handle that in the future, so we can create an issue/discussion point on this.

[rdlrt]

One could also have a perspective that all files that is deployed live should be a version that matches what is committed in the repo

Typically these (for given use case for end users) would be put into deployment or object repository and use submodules instead, but then splitting repository complicates it further too .

I appreciate the hard word went in...and do not doubt the details, but I agree with Tommy in that it could potentially complicate review and PR submission process, and would like to vote for waiting , especially given that we do not have a special feature we'd like to add yet.

It's not to say we will stop upgrade for good, but just to give enough time for consideration in case fixed upstream

[slorber]

Docusaurus maintainer here

Not sure to understand everything but you should be able to run with a docgen step without any error message.

It looks like we have a new bug due to this change: facebook/docusaurus#6593

If you can report it with a smaller repro that would be convenient for us to fix it

[Josh-Cena]

This was also reported on Discord by (probably) another dev. I think it's because nothing is outputted from git log, and we previously had a fail-safe strategy of simply returning no date. I think we should stay fail-safe here.

[katomm]

@slorber I appreciate that you are making the effort to post this here and want to thank you. 🙏🏻 Thanks also @Josh-Cena

[felipecrs]

Do we already know why git log does not return anything? Just trying to understand the environment where the issue runs.

[Josh-Cena]

It's because these files are dynamically generated during build and never tracked by git.

[felipecrs]

@Josh-Cena understood. Thank you. I agree with you at:

This was also reported on Discord by (probably) another dev. I think it's because nothing is outputted from git log, and we previously had a fail-safe strategy of simply returning no date. I think we should stay fail-safe here.

[felipecrs]

But that's exactly what we do already:

https://github.com/facebook/docusaurus/blob/8d1c1954c194c5dd5f2e4c0407679cb3f83dfc96/packages/docusaurus-plugin-content-blog/src/blogUtils.ts#L243

[felipecrs]

So, I think the logger level should be changed to Warning, rather than error.

[felipecrs]

Because, still, this is something meaningful: relying on file creation date for the blog posts is totally misleading (or am I wrong?). The warning would serve the purpose for the user to refactor their generation process so that they add the date in the filename, for example.

[Josh-Cena]

This is for docs, if I'm not mistakened. We also catch there, but I think it's fine if the file doesn't have git history, and we should at most warn once.

[katomm]

That is true, we exclude only a few folders below docs from version tracking (in gitignore):

/docs/governance/cardano-improvement-proposals/*
/static/img/cip/*
/docs/native-tokens/token-registry/*
/docs/get-started/cardano-serialization-lib/*

[felipecrs]

This is for docs, if I'm not mistakened. We also catch there, but I think it's fine if the file doesn't have git history, and we should at most warn once.

Ok, that's fair.

However, there is something missing. If we let it go (as in the past), how the date will be shown in the website? I suspect a dummy date. But, is it really reasonable for production? Should we allow dates to be set as in blog posts?

[Josh-Cena]

However, there is something missing. If we let it go (as in the past), how the date will be shown in the website? I suspect a dummy date. But, is it really reasonable for production? Should we allow dates to be set as in blog posts?

Nothing is shown. The docs don't necessarily need to show last update, as is the case of showLastUpdate*: false

[Josh-Cena]

About explicitly setting a date, it's certainly something we want to do, see facebook/docusaurus#5691 and facebook/docusaurus#5598

[felipecrs]

Got it. Still, plugins that generates documentation files could easily be able to do the date inferring by their selves (by looking at the git history of the files which generated the docs?), and thereafter including the updated/creation date in the generated file (frontmatter?).

[felipecrs]

About explicitly setting a date, it's certainly something we want to do, see facebook/docusaurus#5691 and facebook/docusaurus#5598

Awesome, cool then. I'm going to refactor the code to only show the warning once, and make it a warning instead of error-like (although, currently, it just displays as error and do not stops the execution).

[Josh-Cena]

Still, plugins that generates documentation files could easily be able to do the date inferring by their selves

These are not plugins; they are simply fs.writeFile calls that dynamically write files before releasing control to docusaurus build. I don't think a trivial fix exists here, given the current APIs

[felipecrs]

Still, plugins that generates documentation files could easily be able to do the date inferring by their selves

These are not plugins; they are simply fs.writeFile calls that dynamically write files before releasing control to docusaurus build. I don't think a trivial fix exists here, given the current APIs

I said plugins because I know plugins that does such kind of things, like: https://github.com/tgreyuk/typedoc-plugin-markdown/tree/master/packages/docusaurus-plugin-typedoc.

Some git functions are exposed by the @docusaurus/utils so that either custom build scripts or plugins can leverage what we wrote to find out the update date for a file.

[slorber]

The PR facebook/docusaurus#6937 has been merged

Can you test with the latest canary and see if there's any improvement?

Is the error blocking or just an annoying log?

[katomm]

Thank you so much @slorber. I can confirm it is fine with 0.0.0-4753. (only the "annoying" log)

I'd like to flag that there is still the word "error" in the warning, so you might want to remove that as well. Example:

[WARNING] Error: Failed to retrieve the git history for file "/Users/tommy/Sites/developer-portal/docs/governance/cardano-improvement-proposals/CIP-0024.md" because the file is not tracked by git.

[Josh-Cena]

Yeah, that was caused by a logic error. See facebook/docusaurus#6937 (comment)

In general, anything that doesn't block the build is only meant to alert you, and if you know what's going on, feel free to ignore it even when it literally says "error"😅

[slorber]

👍

Test next canary (facebook/docusaurus#6973 merged), you should now only see a single warning

[katomm]

@slorber single warning confirmed in 0.0.0-4756. This looks very clean now:
[WARNING] Cannot infer the update date for some files, as they are not tracked by git.

(okay, as clean as a warning message can look like)

[katomm]

Closing this in favor of the Docusaurus v2 beta 18 upgrade #572. Thank you all again.