`stripInternal` should work on single-line comments

[parzhitsky]

Bug Report

🔎 Search Terms

stripInternal

🕗 Version & Regression Information

• This is a crash
• This is the behavior in every version I tried, and I reviewed the FAQ for entries about stripInternal

⏯ Playground Link

Playground link with relevant code

💻 Code

/module.ts

/** @internal */
export type Thing = string;

// this is not @internal (using at character)
export type Thing2 = number;

// this is also not internal (not using at character)
export type Thing3 = boolean;

🙁 Actual behavior

/module.d.ts

export declare type Thing3 = boolean;

🙂 Expected behavior

/module.d.ts

export declare type Thing2 = number;
export declare type Thing3 = boolean;

[DanielRosenwasser]

At this point, I don't think it's JSDoc comments. I think the best bet is saying that

• for a single-line // comment, the search string has to be something closer to ^//\s*@internal
• for a multi-line /* */ comment, the search string has to be ^/\*|(?:\n\s*\*)\s*@internal

[parzhitsky]

Regex approach might be unreliable, especially in multi-line case. Can it be implemented via AST node removal? I think, the compiler already parses the code to AST, — can we piggyback on this?

[ValerianClerc]

Hello @DanielRosenwasser! I'd like to tackle this issue, so I'm trying to understand it better. Here are my notes from my investigation:

The existing behaviour (as of 4.9.0) is that given the following input file input.ts, only the notInternal() function will remain in input.d.ts after compilation with -d --stripInternal.

//// [input.ts]
export class TestClass {
  notInternal(): void { }

  // @internal
  internal1(): void { }

  /**
   * @internal
   */
  internal2(): void { }

  // some words here @internal
  internal3(): void { }

  /**
   * some words
   * @internal
   */
  internal4(): void { }

  /* @internal */
  internal5(): void { }

  // @internal
  // something else
  internal6(): void { }

  /**
   *  @internal
   */ 
  // something else
  internal7(): void { }
}

The resulting compilation output (with flags --stripInternal -d):

//// [input.d.ts]
export declare class TestClass {
    notInternal(): void;
}

Currently, we're just searching for any substring @internal within a comment. Here is where this happens:

TypeScript/src/compiler/transformers/declarations.ts

Lines 9 to 12 in bbec17d

	function hasInternalAnnotation(range: CommentRange, currentSourceFile: SourceFile) {
	const comment = currentSourceFile.text.substring(range.pos, range.end);
	return stringContains(comment, "@internal");
	}

The typescript documentation specifies that the @internal annotation must be in a JSDoc comment, but that's not how it is currently implemented. Updating the implementation to only function within JSDoc comments could expose many unwanted types. Even just inside of the Typescript codebase, we often use /* @internal */, which isn't a valid JSDoc comment.

My question: how do you recommend moving forward with this? I see a few options, please let me know your thoughts:

1. Restrict @internal to only JSDoc comments
2. Restrict @internal to only JSDoc comments and multiline comments
3. Restrict @internal so that it must be the first word of the line in a JSDoc/multiline comment (so // some words @internal wouldn't be valid)
4. Do nothing in the code, and instead update the --stripInternal documentation to be more specific/clear, to reflect how @internal is currently implemented

Please let me know your thoughts 😄

[DanielRosenwasser]

Let me check with others on the team. I feel like we've been tripped up by accidentally using single-line comments.