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

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

Closed
Zamiell opened this issue Feb 6, 2023 · 9 comments
Labels
better engineering Not a bug or feature request domain: dx Related to developer experience of working on Docusaurus sites

Comments

@Zamiell
Copy link
Contributor

Zamiell commented Feb 6, 2023

[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: algolia/docsearch#1658 (comment)

@Zamiell Zamiell 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 labels Feb 6, 2023
@Josh-Cena
Copy link
Collaborator

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?

@Josh-Cena Josh-Cena added status: needs more information There is not enough information to take action on the issue. domain: dx Related to developer experience of working on Docusaurus sites and removed status: needs triage This issue has not been triaged by maintainers labels Feb 6, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 6, 2023

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

@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 6, 2023

@Josh-Cena
Copy link
Collaborator

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

@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 6, 2023

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
Copy link
Collaborator

Josh-Cena commented Feb 8, 2023

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.

@Josh-Cena Josh-Cena added external This issue is caused by an external dependency and not Docusaurus. and removed status: needs more information There is not enough information to take action on the issue. labels Feb 8, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Feb 8, 2023

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
Copy link
Collaborator

Josh-Cena commented Feb 8, 2023

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 :)

@Zamiell Zamiell changed the title Algolia Config Interface is not strict Add "@satisfies" JSDoc tag to every "@type" JSDoc tag throughout the repository Feb 8, 2023
@Josh-Cena Josh-Cena added better engineering Not a bug or feature request and removed bug An error in the Docusaurus core causing instability or issues with its execution external This issue is caused by an external dependency and not Docusaurus. labels Feb 8, 2023
@Josh-Cena
Copy link
Collaborator

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

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 domain: dx Related to developer experience of working on Docusaurus sites
Projects
None yet
Development

No branches or pull requests

2 participants