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

[api-extractor] Treat all underscore-prefixed variables as @internal #2133

Open
1 of 2 tasks
schmidt-sebastian opened this issue Aug 21, 2020 · 7 comments
Open
1 of 2 tasks
Labels
needs more info We can't proceed because we need a better repro or an answer to a question

Comments

@schmidt-sebastian
Copy link

schmidt-sebastian commented Aug 21, 2020

Is this a feature or a bug?

  • Feature
  • Bug

Please describe the actual behavior.

Would it be possible to add a setting that automatically treat all underscore-prefixed class members as @internal? If so, we are willing to contribute a PR but I would like to check first.

This would be similar to #1170

@schmidt-sebastian
Copy link
Author

cc @Feiyang1

@schmidt-sebastian schmidt-sebastian changed the title [api-extractor] Treats all underscore prefixed variables as @internal [api-extractor] Treat all underscore-prefixed variables as @internal Aug 21, 2020
@octogonz
Copy link
Collaborator

Could you provide an example source file showing how this would be useful?

@octogonz octogonz added the needs more info We can't proceed because we need a better repro or an answer to a question label Aug 21, 2020
@schmidt-sebastian
Copy link
Author

In the codebases I interact with, we generally prefix the private implementation details of public classes with underscores. This is meant to convey to developers that they should not rely on these members. This convention seems to now be enforced in API Extractor as well (#1170), but we still have to add @internal to our codebase (which we would likely do with a TypeScript transformer if we have to).

A code sample would be: https:/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/geo_point.ts#L32

@octogonz
Copy link
Collaborator

octogonz commented Aug 21, 2020

In the codebases I interact with, we generally prefix the private implementation details of public classes with underscores.

The Rush Stack lint rules follow the same convention, where _ indicates to non-TypeScript developers that the signature is not meant to be accessed by consumers. For the TypeScript language, the policy works out like this:

  • private members -- must always have an underscore (and are generally ignored by API Extractor)
  • protected members -- generally do NOT have an underscore because they are accessible to child classes, which may be created by consumers
  • public members -- generally do NOT have an underscore

API Extractor extends the language's visibility concepts. The @internal tag is for signatures which are accessible (e.g. via export or public or protected), but are not intended to be supported for third parties. Thus @internal can be applied to public or protected members, as well as entire export'ed top-level APIs. In this situation API Extractor ae-internal-missing-underscore provides extra validation beyond the lint rule, requiring that an @internal API should also have an underscore.

export class GeoPoint {
  // Prefix with underscore to signal this is a private variable in JS and
  // prevent it showing up for autocompletion when typing latitude or longitude.
  private _lat: number;
  private _long: number;

Your example here is private, so it does not need to be marked as @internal.

@octogonz
Copy link
Collaborator

It sounds like your intended case is more like this:

/**
 * Renders a UI element.
 * @public
 */
export class Widget {
  /** Draw the thing */
  public render(): void;

  /** Return the internal implementation details */
  public _getImplementation(): IWidgetInternal;
}

Today API Extractor expects you to explicitly add an @internal tag like this:

/**
 * Renders a UI element.
 * @public
 */
export class Widget {
  /** Draw the thing */
  public render(): void;

  /** 
   * Return the internal implementation details 
   * @internal
   */
  public _getImplementation(): IWidgetInternal;
}

It would save some coding effort to automatically interpret any underscore-prefixed member as a @internal. But in being less explicit, it might be less clear to users that _getImplementation was meant to be internal. For professionally supported APIs, generally the developers will put careful thought into what is-or-is-not meant to be part of the supported contract.

Another idea would be to consider issuing a warning when _getImplementation is public and prefixed by an underscore. Was that a mistake? If not, maybe the developer should be required to include an explicit @public tag to indicate that the underscore was intentional.

@schmidt-sebastian
Copy link
Author

Sorry, that was indeed a bad example. We are also using "_" prefixes for APIs that are only public because we need to access them from a different module, which essentially works around the fact that TypeScript doesn't have package-group (e.g @firebase/*) visibility. We have a fairly complex codebase, and this not an uncommon pattern in our SDK. As I was starting to add the @internal keyword in the PR that enables API Extractor, it seemed like this task could be automated.

As for "accidental underscores": We rely on our users to not use underscore-prefixed variables. Since we require this understanding from our users, I hope that I can also require this from my team and our contributors.

I do think that this should be optional behavior though, as it will likely surprise (and break) some users

@k8w
Copy link

k8w commented Mar 22, 2021

Maybe it is better to make private members be treated as @internal by default?
Cuz in most cases, private members are not expected to be exported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info We can't proceed because we need a better repro or an answer to a question
Projects
Status: AE/AD
Development

No branches or pull requests

3 participants