Skip to content

Conversation

@hamsa-s
Copy link
Contributor

@hamsa-s hamsa-s commented Nov 21, 2021

This fixes issue #13617. The new linter rule checks that private members don't use the internal tag in the TSdoc comments.

Here are the steps I took to solve the issue and write the new rule:

  • I implemented the new rule in ts-doc-internal-private-member.ts to check if any private members use the internal tag in the TSdoc comments. I based this off of ts-doc-internal.ts since its purpose is similar but added checks for private members for the new rule
  • I wrote appropriate tests for the rule which are placed tests/rules
  • I added the documentation file to docs/rules
  • I updated src/configs/index.ts, src/rules/index.ts, tests/plugin.ts, and README.md to add this new rule in the linter
  • I rebuilt the eslint package and ran rush lint on the entire repository
  • I fixed any errors in packages due to the new rule

Please let me know if there is anything I missed or anything else I should do since I'm very new to this

@ghost ghost added eslint plugin customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Nov 21, 2021
@ghost
Copy link

ghost commented Nov 21, 2021

Thank you for your contribution hamsa-s! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Nov 21, 2021

CLA assistant check
All CLA requirements met.

@jeremymeng
Copy link
Member

  1:1  error  Resolve error: RangeError: Maximum call stack size exceeded
    at Hash.update (internal/crypto/hash.js:74:40)
    at hashObject (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:45:8)
    at hashify (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:18:5)
    at /workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:49:5
    at Array.forEach (<anonymous>)
    at hashObject (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:46:30)
    at hashify (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:18:5)
    at hashArray (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:32:5)
    at hashify (/workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:16:5)
    at /workspaces/jssdk/common/temp/node_modules/.pnpm/[email protected]/node_modules/eslint-module-utils/hash.js:49:5  import/no-extraneous-dependencies

It seems like it's either some deeply nested object with array properties, or an object with circular reference that caused infinite recursive calls.

@jeremymeng
Copy link
Member

Add @deyaaeldeen @witemple-msft in case either of you have any idea. The error message has import/no-extraneous-dependencies but I don't see how it is involved in this PR.

@deyaaeldeen
Copy link
Member

@jeremymeng this error could happen if the import rule is going through a circular import statement but I am not sure how could this happen in this PR.

@jeremymeng jeremymeng added the Impact++ This pull request was submitted by a member of the Impact++ team. label Dec 8, 2021
@jeremymeng
Copy link
Member

Looking more closely, I think we are missing the reporting of the private members. We should be reporting when @internal is used in the doc of private class members https://www.typescriptlang.org/docs/handbook/2/classes.html#private

export class B {
    /**
     * @internal
     */
    private x = 0;

    /**
     * @internal
     */
    private get getter(): number { return 0 }

  // other members.
}

Having private on classes or functions are not valid TypeScript syntax so we can ignore those.

I found the following member nodes can have a visibility modifier, out of the possible TS syntax nodes:

  • ClassPropertyBase
    • the doc recommends using ClassPropertyComputedNameBase/ClassPropertyNonConComputedNameBase instead
  • MethodDefinitionBase
    • the doc recommends using MethodDefinitionComputedNameBase/MethodDefinitionNonComputedNameBase instead
  • TSMethodSignatureBase
  • TSPropertySignatureBase
  • TSIndexSignature
  • TSPrameterProperty

I think we can match these nodes then checking for their accessibility and jsDoc. This is my current understanding of the original issue.

Do we want to report the cases when @internal appears in the doc of top-level class/functions that are not exported? Maybe not as this seems to be in conflict with the ts-doc-internal rule where we require @internal or @hidden for things that are not exported?

Thoughts? @deyaaeldeen

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! I have a few comments but overall it's good!

Copy link
Member

@witemple-msft witemple-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I have a couple of code style comments below.

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than two minor comments!

@deyaaeldeen @witemple-msft any additional feedback?

@deyaaeldeen
Copy link
Member

@jeremymeng this looks great! Thanks a lot @hamsa-s!

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you very much for your contribution!

@jeremymeng jeremymeng enabled auto-merge (squash) February 2, 2022 00:51
@jeremymeng jeremymeng merged commit 1dbd41d into Azure:main Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. eslint plugin Impact++ This pull request was submitted by a member of the Impact++ team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants