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

[V3] Improved Docusaurus 3 compatibility #677

Merged
merged 6 commits into from Dec 13, 2023

Conversation

Gijsdeman
Copy link

@Gijsdeman Gijsdeman commented Dec 6, 2023

Description

Improved escaped descriptions and headers, introduced typing for sidebars.

Motivation and Context

Fixes some of the issues mentioned in #654:

The first two items are pretty much required for a proper Docusaurus 3 support. The last one introduces typed sidebars which has improved support in Docusaurus 3.

There is one issue which I am unsure to fully resolve. HTML sidebar items are broken in Docusaurus 3.0.0 and hence an upgrade to 3.0.1 was needed to make sure versioned sidebars function as expected. However, this now introduces an Hook is called outside the <DocProvider> error, which I am unable to replicate in other projects (this error seems to be caused by the @theme/ApiItem in the docusaurus config file).

How Has This Been Tested?

Tested by hand, using personal OpenAPI specifications.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@sserrata
Copy link
Member

sserrata commented Dec 6, 2023

There is one issue which I am unsure to fully resolve. HTML sidebar items are broken in Docusaurus 3.0.0 and hence an upgrade to 3.0.1 was needed to make sure versioned sidebars function as expected. However, this now introduces an Hook is called outside the error, which I am unable to replicate in other projects (this error seems to be caused by the @theme/ApiItem in the docusaurus config file).

Hi @Gijsdeman thanks for the PR, it's likely Docusaurus introduced some breaking change in 3.0.1. Will need to debug to determine the root cause.

@Gijsdeman
Copy link
Author

Gijsdeman commented Dec 8, 2023

I found out that I forgot to update some of the Docusaurus dependencies to 3.0.1. I think this caused a version mismatch that caused the above-mentioned error.

Updating these dependencies fixed the problem. Also, the versioned API documentation sidebar now works correctly.

@Gijsdeman
Copy link
Author

@sserrata I also looked into updating the demo to make full use of the improved TypScript support of Docusaurus 3.

Docusaurus indicates that TypeScript 5.0 is required, but this caused quite some dependency issues. Instead, I opted to use TypeScript 4.9.5, and updated eslint and some eslint-plugins. This all works fine and does not cause any issues as far as I can see.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @Gijsdeman was this possibly checked in by mistake?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry. Let me remove it from the commit and fix the prettier issues.

@sserrata
Copy link
Member

sserrata commented Dec 8, 2023

@Gijsdeman concerning the latest jest error, you should be able to address it by updating the test script in package.json as follows:

    "test": "NODE_OPTIONS=--experimental-vm-modules jest"

After that, run yarn test -u to update the snapshot and it should work.

@Gijsdeman
Copy link
Author

Thanks. For some reason I also had to change the createSchema.test.ts as it was returning an unresolved Promise. This came from the prettier.format, even though this does not have async function type.

Copy link

github-actions bot commented Dec 8, 2023

Visit the preview URL for this PR (updated for commit 9381c8b):

https://docusaurus-openapi-36b86--pr677-kuhyatrz.web.app

(expires Fri, 12 Jan 2024 00:25:11 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if upgrading packages introduced a change to our eslint/prettier rules? Before, we were not requiring comma dangle. It's not a huge issue but it is changing a lot of files unnecessarily.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I can add "trailingComma" : "none" to .prettierrc.json to keep the behaviour as it was before.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, thanks so much for all the effort - really appreciated 🙏🏽 !

Copy link
Author

Choose a reason for hiding this comment

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

I think this actually made it worse. I do not think prettier enforced this rule at all in the past, so either way around there will be a lot of files changed.

I think "trailingComma": "es5" might be what we want, but I will have to look at that again next week. Sorry!

Copy link
Member

Choose a reason for hiding this comment

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

I'll investigate further if I have time today. Either way, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Trailing Commas: Default value changed from es5 to all in v3.0.0

I changed and indeed no files are changed because of trailing commas, which indeed caused a lot less files to be changed due to the prettier upgrade (in total 15 files compared to the ~90 earlier).

It looks like these changes are not due to the trailing comma, but due to some other minor changes in prettier (see packages/docusaurus-theme-openapi-docs/src/theme/ParamsItem/index.js and packages/docusaurus-theme-openapi-docs/src/theme/ApiExplorer/Response/_Response.scss for example).

@sserrata
Copy link
Member

Hi @Gijsdeman, just finished stepping through the recent changes and reviewed the deploy preview. LGTM!

@sserrata sserrata merged commit 20a21b0 into PaloAltoNetworks:v3.0.0 Dec 13, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants