fix(v2): Added back support for optional logo field in theme-classic navbarConfig

[SamChou19815]

Motivation

Fix #3610.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Comment out the logo field in docusaurus website's docusaurus.config.js, like:

  // ...
  navbar: {
      hideOnScroll: true,
      title: 'Docusaurus',
      /* logo: {
        alt: 'Docusaurus Logo',
        src: 'img/docusaurus.svg',
        srcDark: 'img/docusaurus_keytar.svg',
      },
      */
  // ...
  }

Without this change, site will crash:

With this change, site will load as usual, without the logo:

Related PRs

N/A

[lex111]

Maybe we can make the logo object optional in the validation scheme?

docusaurus/packages/docusaurus-theme-classic/src/validateThemeConfig.js

Lines 245 to 251 in 2e7aef4

	logo: Joi.object({
	alt: Joi.string().allow(''),
	src: Joi.string().required(),
	srcDark: Joi.string(),
	href: Joi.string(),
	target: Joi.string(),
	}),

[docusaurus-bot]

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit bf593e0

https://deploy-preview-3611--docusaurus-2.netlify.app

[SamChou19815]

Maybe we can make the logo object optional in the validation scheme?

docusaurus/packages/docusaurus-theme-classic/src/validateThemeConfig.js

Lines 245 to 251 in 2e7aef4

	logo: Joi.object({
	alt: Joi.string().allow(''),
	src: Joi.string().required(),
	srcDark: Joi.string(),
	href: Joi.string(),
	target: Joi.string(),
	}),

I think it's already optional here, since there is no .required() for this object, and my build failed with SSR error due to this bug instead of validation error.

@lex111 Oh I see your point. I have moved the default value to the config validation code.

[SamChou19815]

Reverted to the old logo = {} approach in useLogo.ts. Making {} as default value doesn't work since the validator expects logo.src to be nonnull.

[lex111]

Hmm, wouldn't the optional() work in our case?

 logo: Joi.object({ 
   alt: Joi.string().allow(''), 
   src: Joi.string().required(), 
   srcDark: Joi.string(), 
   href: Joi.string(), 
   target: Joi.string(), 
 }).optional(), 

[SamChou19815]

Hmm, wouldn't the optional() work in our case?

 logo: Joi.object({ 
   alt: Joi.string().allow(''), 
   src: Joi.string().required(), 
   srcDark: Joi.string(), 
   href: Joi.string(), 
   target: Joi.string(), 
 }).optional(), 

I think optional is already the default, so adding optional doesn't help at all.

[slorber]

My mistake, didn't see this code could fail.

@SamChou19815 the idea of the PR was to remove all the default values from the theme, so I'd rather avoid reintroducing "logo = {}" in the theme.

The following does not work?

 logo: Joi.object({ 
   alt: Joi.string().allow(''), 
   src: Joi.string().required(), 
   srcDark: Joi.string(), 
   href: Joi.string(), 
   target: Joi.string(), 
 }).default({}), 

Afaik the default value is not run against the validators by Joi

[SamChou19815]

@slorber I tried this approach in the first few commits, but it failed tests.

Currently, the src field in logo field is required. If we simply add .default({}). It will complain about src missing.

docusaurus/packages/docusaurus-theme-classic/src/validateThemeConfig.js

Lines 245 to 251 in 2e7aef4

	logo: Joi.object({
	alt: Joi.string().allow(''),
	src: Joi.string().required(),
	srcDark: Joi.string(),
	href: Joi.string(),
	target: Joi.string(),
	}),

If we want to add .default({}), we also need make src field optional, which is what the client code in useLogo.ts implicitly implied and it doesn't really make sense.

[slorber]

Ok so let's merge it this way for now, and we'll figure out how to refactor this useLogo hook later