facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.14k stars 8.42k forks source link

Add "@satisfies" JSDoc tag to every "@type" JSDoc tag throughout the repository #8639

Closed Zamiell closed 2 months ago

Zamiell commented 1 year ago

[edit - Original issue was about specifying bogus fields in my "docusaurus.config.js" file and it not throwing any errors.]

Description

This is nice, because it allows for auto-complete and in-editor validation when adding entries to your configuration file. However, this interface is not strict insofar that you can add bogus entries to the Algolia config without getting an error from the TypeScript compiler. This is a mistake, as we surely want to alert users with a red squiggly line in their editor (e.g. VSCode) the moment that they make a typo here (or insert a non-valid entry).

Motivation

I wasted months troubleshooting issues with Algolia due to Docusaurus essentially lying to me about what were valid configuration entries: https://github.com/algolia/docsearch/issues/1658#issuecomment-1419764650

Josh-Cena commented 1 year ago

By "not strict", do you mean we mark things as optional? Or do you mean you can add properties that do not match the declared type?

Zamiell commented 1 year ago

I mean that end-users can add properties that do not match any of the valid declared types.

Zamiell commented 1 year ago

e.g. When working in a Docusaurus config, I expect that most people will expect an error like this: https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgGIHt3IN4ChnIBucANgK4QCMAXMiGQLYBG0A3PkaRQEy33NtcAX1y4E6EAGcwyGJloYsAXhwdi5KrUrcAzABo1XCL2QAWAKwA2AwXUUdtAOwAOAJwGRQA

Josh-Cena commented 1 year ago

Do you have a repro? When you hover over the algolia key and go to definition, what do you see?

Zamiell commented 1 year ago

When you hover over the algolia key and go to definition, what do you see?

When I hit F12 on the algolia key, it just takes me to theme-search-algolia.d.ts file, which looks like this:

  export type ThemeConfig = {
    algolia: {
      contextualSearch: boolean;
      externalUrlRegex?: string;
      appId: string;
      apiKey: string;
      indexName: string;
      searchParameters: {[key: string]: unknown};
      searchPagePath: string | false | null;
      replaceSearchResultPathname?: {
        from: string;
        to: string;
      };
    };
  };

The corresponds to this file, which I already included in the OP: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-search-algolia/src/theme-search-algolia.d.ts#L11

Do you have a repro?

Does the bug not happen for you? If you want, you can clone my repository and see for yourself: https://github.com/IsaacScript/isaacscript

Josh-Cena commented 1 year ago

This is working as intended. We are using /** @type {import('@docusaurus/preset-classic').ThemeConfig} */ to type the themeConfig property, which is not the same as a type annotation. In fact, @type is closer to an as assertion. TypeScript's excess property check is very limited and only works in very certain cases where it's sure that "something can't be accessible", and even if it doesn't error, the result is not unsound anyway, since TS is structurally typed. Here's a dumbed-down case:

Playground

/** @typedef {{
  value1: number;
  value2: number;
}} Foo */

const foo = {
  prop: /** @type {Foo} */ ({
    value1: 2,
    value2: 1,
    value3: 3,
  })
};

This will be fixed in TS 5.0 by using the @satisifies annotation, but before that, there's little we can do.

Zamiell commented 1 year ago

Ah, I see.

Well then maybe we can turn this into an issue for tracking the adding of the @satisfies JSDoc tag everywhere once TypeScript 5.0 releases. (Unless there's another issue for it?)

Another good option is to have the config file be generated in TypeScript from the get-go, at least when the --typescript flag is used to bootstrap the website. I see that that is being tracked in #7911, so I'll eagerly await either (or both) of these upgrades.

Josh-Cena commented 1 year ago

Yes, we'll probably close this by changing to @satisfies. And yes, before #7911, we can't support TS configs, so using @satifies is our best mitigation.


Well actually, even if you use TS, you can't type an object embedded within another object without using satisfies. In JSDoc, the equivalent to using a type annotation:

const foo: Foo = {...};

Is to also use @type, but apply it to a variable instead of an expression:

/** @typedef {{
  value1: number;
  value2: number;
}} Foo */

/** @type {Foo} */
const foo = {
  value1: 2,
  value2: 1,
  value3: 3,
};

And this works. So there's nothing unique about JSDoc either, it's just neither of us know JSDoc much :)

Josh-Cena commented 2 months ago

We now have TS configs so if you want strict type validations you should use that. Closing this as resolved!