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

Cleanup themes default/fallback values #3342

Closed
slorber opened this issue Aug 27, 2020 · 3 comments · Fixed by #3394
Closed

Cleanup themes default/fallback values #3342

slorber opened this issue Aug 27, 2020 · 3 comments · Fixed by #3394
Labels
better engineering Not a bug or feature request bug An error in the Docusaurus core causing instability or issues with its execution difficulty: starter Issues that are starter difficulty level, e.g. minimal tweaking with a clear test plan. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR.

Comments

@slorber
Copy link
Collaborator

slorber commented Aug 27, 2020

image

https://twitter.com/jaredpalmer/status/1298332787792109570

With the new theme validation/normalization, default values are applied ahead of time.

We should cleanup every piece of code that defines default/fallback values in the views unnecessarily because we don't want these fallback values to be duplicated everywhere, and it pollutes the code uselessly.

const {
  siteConfig: {
    themeConfig: { announcementBar: { id = "annoucement-bar" } = {} } = {}
  } = {}
} = useDocusaurusContext();

Here, we know that there's always a siteConfig, a themeConfig, and maybe an announcement bar, so it's not necessary to use = {} each time.

Also, we should strive to have proper TypeScript support. It is possible to have no announcementBar config, but I don't think we should destructure that config, falling back to {} just to ensure a runtime error.

In such case, the correct code should rather be:

const { announcementBar } = useDocusaurusContext().siteConfig.themeConfig;
if (announcementBar) {
  const {id,content} = announcementBar;
}

This announcementBar case has been fixed with #3338

But there are many other places where such cleanup should be done.

I suggest we should also:

  • Add more config normalization tests (as Joi is not always so idiomatic)
  • Add a useThemeConfig hook to reduce a bit boilerplate
@slorber slorber added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers better engineering Not a bug or feature request difficulty: starter Issues that are starter difficulty level, e.g. minimal tweaking with a clear test plan. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR. and removed status: needs triage This issue has not been triaged by maintainers labels Aug 27, 2020
@imskr
Copy link
Contributor

imskr commented Aug 27, 2020

@slorber I would like to start working on this!

@slorber
Copy link
Collaborator Author

slorber commented Aug 28, 2020

Please do @imskr :)

@imskr
Copy link
Contributor

imskr commented Sep 3, 2020

@slorber I have made a PR #3394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
better engineering Not a bug or feature request bug An error in the Docusaurus core causing instability or issues with its execution difficulty: starter Issues that are starter difficulty level, e.g. minimal tweaking with a clear test plan. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants