refactor(theme-classic): completely migrate package to TypeScript

[Josh-Cena]

Motivation

Although this package is almost all TS, there are a few JS files + untyped dependencies. This PR completes the TS infrastructure, and thus removed the noImplicitAny: false option in tsconfig.json.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

CI

Related PRs

Duplicated work with #5442, should merge that first

[netlify]

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 288f191

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

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

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

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

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

[Josh-Cena]

As I understand PascalCased variables should be objects right? (Also noticed this in docusaurus-init where TypeScriptTemplateSuffix is PascalCased)

[slorber]

I don't have a strong opinion about JS constant namings as long as it's clear enough those are constants. We already have this convention in other places so just made it consistent, but we can also redefine those conventions if needed

[slorber]

LGTM thanks 👍

Also refactored a bit the versionBanner prop because handling 'none' with Exclude was not very convenient

[Josh-Cena]

Yeah, I was thinking about using null as well, but not sure if that would break any exposed API

[Josh-Cena]

Slightly off-topic: I prefer removing inferable types wherever possible, and it also seems to be the case for the majority of TS code across the repo, except for docusaurus-core where I spotted a lot of redundant annotation. However it seems there isn't an ESLint rule to enforce this 🤷‍♂️

[slorber]

I feel inferable types have their place in a codebase to avoid some useless duplication (cf highlighted languages using as const), as long as the public API surface is not inferred (ie API changes must be explicit when reviewing code). It's probably worth discussing this on a case-by-case basis.

[Josh-Cena]

the public API surface is not inferred

This is taken care of by the explicit-boundary-types rule which I find reasonable. I was just talking about no-inferable-types rule being too limited in its scope (only catching the really "trivial" types like const a: string = 'Hello')... There are some feature requests on making a no-unnecessary-annotation rule but seems no implementation yet

[slorber]

I don't always agree with that, extra type annotations on intermediate variables can significantly help readability and also help split a complex data processing algo in multiple "steps" in which you can more easily see which step doesn't typecheck (instead of having the full algo not typechecking). I'd only remove types on a case-by-case basis and limit to the trival types inferences only.

[slorber]

A good example where an algo can be seen as a concatenation of 2 steps, where explicit (but unnecessary) types can help figure out which step fails to typecheck:

function collectSidebarItemsOfType<
  Type extends SidebarItemType,
  Item extends SidebarItem & {type: SidebarItemType}
>(type: Type, sidebar: Sidebar): Item[] {
  function collectRecursive(item: SidebarItem): Item[] {
    const currentItemsCollected: Item[] =
      item.type === type ? [item as Item] : [];

    const childItemsCollected: Item[] =
      item.type === 'category' ? flatten(item.items.map(collectRecursive)) : [];

    return [...currentItemsCollected, ...childItemsCollected];
  }

  return flatten(sidebar.map(collectRecursive));
}

[Josh-Cena]

Yeah, this one makes sense 👍 Thanks for the MWE