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

In typescript mode, only allow {[key: string]: string} and not object<>, object.<>, Object<> and Object.<> #1001

Closed
thernstig opened this issue Mar 21, 2023 · 9 comments · Fixed by #1012

Comments

@thernstig
Copy link

thernstig commented Mar 21, 2023

Motivation

TypeScript Do's and Dont's (https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#general-types) says to never use Object (capital O). So when in typescript mode, only ever object should be allowed as default, and not Object.

The reason is that a ton of users in our project always incorrectly uses Object in JSDoc, when it pretty much always should be object since we use typescript mode, which causes a lot of unnecessary code review comments.

This was fixed in #800.

However, since jsdoc-type-pratt-parser/jsdoc-type-pratt-parser#101 has been fixed, object<>, object.<>, Object<> and Object.<> should be disallowed as well.

One can use {[key: string]: string} instead when in typescript mode. This should thus be the default.

Current behavior

When eslint-plugin-jsdoc is set to "mode": "typescript", then both object<>, object.<>, Object<> and Object.<> is allowed.

Desired behavior

When "mode": "typescript" is set, only {[key: string]: string} should be allowed by default.

Alternatives considered

I could define this setting:

  "settings": {
    "jsdoc": {
      "mode": "typescript",
      "preferredTypes": {
        "Object": "object",
        "Object<>": false,
        "Object.<>": false,
        "object<>": false,
        "object.<>": false
      }
    }
  }

But the problem with that is that pretty much anyone using typescript mode would have to use that setting. It'd be much better to change the default, as that is an absolute "do and don't" to follow.

Those who want to allow object<>, object.<>, Object<> and Object.<> etc. can already do that via preferredTypes.

@thernstig thernstig changed the title In typescript mode, only allow {s: string, n: number} and not object<>, object.<>, Object<> and Object.<> In typescript mode, only allow {[key: string]: string} and not object<>, object.<>, Object<> and Object.<> Mar 21, 2023
@brettz9
Copy link
Collaborator

brettz9 commented Apr 7, 2023

Regarding the {[key: string]: string} syntax, do you mean only allowing any object syntax like {a: string, b: number}, {[key: string]: number}, etc.? The only problem with this is that some projects might prefer to have a single line per property, both because different properties may have different types, but also so as to provide a distinct text description for each object. "object" may currently often be used more for roots like:

/**
 * @param {object} cfg
 * @param {number} cfg.val Some desc.
 * @param {string} cfg.anotherVal Another desc.
 */
function quux ({val, anotherVal}) {}

But we could certainly forbid most of the others by default for TS, I think.

@thernstig
Copy link
Author

thernstig commented Apr 8, 2023

@brettz9 I mean to disallow these:

      "preferredTypes": {
        "Object<>": false,
        "Object.<>": false,
        "object<>": false,
        "object.<>": false
      }

Those are JSDoc type syntaxes, that TypeScript does not have. See https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#types-1.

So when in typescript mode, they should not be used. Instead, index signatures (or Record in some cases) should be used. Support was recently added in pratt-parser for index signatures {[key: string]: string}.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 8, 2023

Understand, but I just want to be clear, you still want to allow plain object and other forms like {a: string} in addition to index signatures, right?

Also, FWIW, TS does technically allow at least the Object-based forms (and they have a long-standing issue to support the object based ones: microsoft/TypeScript#20555 ), like the example you posted does support Object.<>. I am aware Object use is discouraged, but just mentioning.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 8, 2023

Btw, I'm just not so sure that, although getting rid of even Object<> does seem more in the spirit of the rule (avoiding the native types), it does seem to come at a cost that {[key: string]: string} is a little uglier than Object<string, string>. Any thoughts about that?

@thernstig
Copy link
Author

Agree it is uglier, at the start. But when getting used to it, it is not anymore. It is very subjective.

The main goal of some projects I would suspect would be to go from pure JSDoc types to TypeScript types and then finally to pure TypeScript using plugin:jsdoc/recommended-typescript.

I.e. the current JSDoc support TypeScript has, together with your awesome ESLint plugin, is that it gives a great incremental route towards pure TypeScript. Which can take years on large projects if done bit by bit.

So forcing to use pure TypeScript types in JSDoc makes this easier to transition in the end since one can just copy&paste the types.

Hope it made sense. But of course, it is up to you. I do think forcing index signatures goes into the spirit of using "typescript" mode.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 9, 2023

Yes, and it makes sense. Even if not transitioning, with Object<> not working as a TypeScript type, non-TS users may become confused when using TS. And again, the rule has long been discouraging specifying native types since it gives the impression that only Object-inherited objects may be used. So I'm leaning to accept it, but some projects might feel it is ok to use it since TS does allow for it within JSDoc. Wonder if there are any other opinions among others out there?

@thernstig
Copy link
Author

@brettz9 if you change the default, users who still prefer the old way can always set preferredTypes right? So that covers their case.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 10, 2023
…res; fixes gajus#1001

BREAKING CHANGE:

For typescript mode, one must use object shorthand or index signatures, e.g., `{[key: string]: number}` or `{a: string, b: number}`
@brettz9
Copy link
Collaborator

brettz9 commented Apr 10, 2023

You might want to look at #1012 with its test examples and docs to see if it matches your understanding.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 10, 2023
…res; fixes gajus#1001

BREAKING CHANGE:

For typescript mode, one must use object shorthand or index signatures, e.g., `{[key: string]: number}` or `{a: string, b: number}`
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 11, 2023
…res; fixes gajus#1001

BREAKING CHANGE:

For typescript mode, one must use object shorthand or index signatures, e.g., `{[key: string]: number}` or `{a: string, b: number}`
brettz9 added a commit that referenced this issue Apr 11, 2023
…res; fixes #1001

BREAKING CHANGE:

For typescript mode, one must use object shorthand or index signatures, e.g., `{[key: string]: number}` or `{a: string, b: number}`
@github-actions
Copy link

🎉 This issue has been resolved in version 41.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants