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

feat: properly type-check the Docusaurus config of new sites #5589

Merged
merged 13 commits into from
Sep 30, 2021

Conversation

bmiddha
Copy link
Contributor

@bmiddha bmiddha commented Sep 19, 2021

PR created by @bmiddha, completed by @slorber

Motivation

  • Introduce new types for user-provided Config, ThemeConfig and plugin Options (raw, unnormalized types)
  • Make Config consistent with API reference and expectations.
  • Add // @ts-check for all new sites (+ Docusaurus site)
  • Docusaurus site should typecheck its config in CI and watch mode
  • Fix existing site typecheck issues that were previously unreported
  • Make Config consistent with API reference and expectations.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Site config errors should be reported on yarn workspace website typecheck

CI should pass, ensuring our own site config is valid

Related Issues

closes #5588

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 19, 2021
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Sep 19, 2021

Remember there are two configs: one user-provided, and one after Joi normalization. The code expects the one after normalization where all these keys actually exist, therefore the current fix doesn't work. I think a more sensible fix is to provide another UserDocusaurusConfig type? In fact the same thing happens for Navbar and Footer configs, preventing me from writing my config in TS

@bmiddha bmiddha closed this Sep 19, 2021
@bmiddha bmiddha reopened this Sep 19, 2021
@bmiddha
Copy link
Contributor Author

bmiddha commented Sep 19, 2021

I like the UserDocusaurusConfig idea.
Something like this perhaps?

export interface UserDocusaurusConfig extends Partial<DocusaurusConfig> {
  baseUrl: string;
  url: string;
  title: string;
}

@Josh-Cena
Copy link
Collaborator

That looks good to me. Are you sure the required fields include and only include these keys?🧐

@bmiddha
Copy link
Contributor Author

bmiddha commented Sep 19, 2021

Seems like it. I based that from the docusaurus.config.js reference.
From a brief look at packages\docusaurus\src\server\configValidation.ts, baseUrl title, and url should be the minimum config required.

@netlify
Copy link

netlify bot commented Sep 19, 2021

✔️ [V2]

🔨 Explore the source changes: 0538186

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6155d6e2556ccf000841173c

😎 Browse the preview: https://deploy-preview-5589--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Sep 19, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 90
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5589--docusaurus-2.netlify.app/

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

UserDocusaurusConfig is nice but this is public API surface that we should also add in our doc and init templates JSDoc annotations.

Isn't it confusing to for users to have UserDocusaurusConfig (in config) + DocusaurusConfig (in code, pages etc...)?

There are other naming possibilities like DocusaurusConfig + DocusaurusConfigNormalized, which is better?

Is there another better alternative?

Since this is a regular pattern to have partial user inputs that get normalized, we should find and stick to a shared convention for type names, particularly when they are documented as public API like in this case.

packages/docusaurus-types/src/index.d.ts Outdated Show resolved Hide resolved
@Josh-Cena
Copy link
Collaborator

Isn't it confusing to for users to have UserDocusaurusConfig (in config) + DocusaurusConfig (in code, pages etc...)?

Not many users use the DocusaurusConfig type explicitly, unless there are use-cases I didn't think of.

There are other naming possibilities like DocusaurusConfig + DocusaurusConfigNormalized, which is better?

DocusaurusConfigNormalized is too long even for an internal type, I'd rather want to save a few keystrokes.

Not saying UserDocusaurusConfig is a good name, but just some thoughts

@slorber
Copy link
Collaborator

slorber commented Sep 22, 2021

Not many users use the DocusaurusConfig type explicitly, unless there are use-cases I didn't think of.

It's in the JSDoc template so it's in a few newly initialized sites already

I'm fine with UserDocusaurusConfig, I don't have better either 🤪 naming is hard

@Josh-Cena
Copy link
Collaborator

It's in the JSDoc template so it's in a few newly initialized sites already

I meant the actual DocusaurusConfig with more required fields than UserDocusaurusConfig :P If we only expose UserDocusaurusConfig there won't be much confusion, although the name is kind of weird on its own

@slorber
Copy link
Collaborator

slorber commented Sep 22, 2021

DocusaurusConfig type will remain exposed because it's provided as props and useDocusaurusContext() return anyway, so both are part of the public AP. For TS sites, users may need to import both types, that's why we should try to make things not too confusing 🤪

# Conflicts:
#	packages/docusaurus-plugin-content-blog/package.json
#	packages/docusaurus-preset-classic/src/preset-classic.d.ts
#	packages/docusaurus-theme-common/package.json
#	packages/docusaurus-theme-common/src/utils/useThemeConfig.ts
@slorber
Copy link
Collaborator

slorber commented Sep 30, 2021

FYI I used the export Config: it will be a simpler export name and be consistent with Options and ThemeConfig.

Using User prefix everywhere is a bit heavy from a public API perspective, I'd rather have a Docusaurus prefix for the normalized version

@slorber slorber changed the title fix: makes types DocusaurusConfig optional to match docs feat: typecheck the Docusaurus config Sep 30, 2021
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Sep 30, 2021
website/docs/blog.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

PR is ready for me and we have better config typechecking running in our CI now

Just not sure why some Prettier changes merged recently are reverted 😅

@Josh-Cena
Copy link
Collaborator

For weird reasons my IDE still reformats the old way on save 😓

Could it be a local version incompatibility? I suppose you have run yarn install, so maybe you have an older version of Prettier installed globally? (Don't know how Idea does the formatting)

@slorber
Copy link
Collaborator

slorber commented Sep 30, 2021

After restarting Intellij, it seems fixed :)

@slorber slorber merged commit 09550b0 into facebook:main Sep 30, 2021
@slorber slorber changed the title feat: typecheck the Docusaurus config feat: properly type-check the Docusaurus config of new sites Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 DocusaurusConfig required types don't match documentation
4 participants