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-documenter] Generated file names for constructors #1303

Open
acchou opened this issue May 28, 2019 · 4 comments
Open

[api-documenter] Generated file names for constructors #1303

acchou opened this issue May 28, 2019 · 4 comments
Labels
ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon.

Comments

@acchou
Copy link
Contributor

acchou commented May 28, 2019

In some recent version of api-documenter (not exactly sure which version), markdown output files for constructors started to appear, and the filenames contain parentheses. E.g.:

docs/api/faastjs.statistics.(constructor).md

This appears fine to markdown but when run through other processing tools like Docusaurus these filenames appear to be not recognized by the markdown link => URL link transformation somewhere in the guts of the markdown to HTML transformer.

A concrete example is this class in the faast.js documentation: https://faastjs.org/docs/api/faastjs.statistics. The constructor link is broken because it retains the ".md" extension. Changing the URL to remove ".md" shows the correct page is accessible.

While this is likely a bug somewhere in Docusaurus or its dependencies, I think it's a bit unwise to generate markdown files with paren characters because they can be mishandled by downstream tools like this one.

@octogonz octogonz added the enhancement The issue is asking for a new feature or design change label May 29, 2019
@octogonz
Copy link
Collaborator

I agree. I noticed that when we merged PR #1295, but it's not trivial to solve, so I didn't want to block the feature over it.

For a little background, the technical issue is that in TypeScript, the word "constructor" is a different kind of name than a member name. You can see this fact in the following example:

class X {
  public x: number;
  public constructor(x: number) {
    this.x = x;
  }
  public "constructor"(): string {
    return `The number is ${this.x}`;
  }
}

let x = new X(123);
// Output: "The number is 123"
console.log(x.constructor());

It is a part of family of constructs such as new (the construct signature operator) and stuff that doesn't have any natural name at all, for example:

interface LocationFormatter {
  /**
   * A function call that formats a 2D vector as a text string.
   */
  (x: number, y: number): string;

  /**
   * A function call that formats a 3D vector as a text string.
   */
  (x: number, y: number, z: number): string;

  /**
   * (Some other hypothetical function in this example.)
   */
  call(): void;
}

Note that TSDoc declaration references already provide a string ID that uniquely identifies each of these. But we need to come up with a way to map these IDs onto a URL using safe characters (and it also needs to be case-insensitive, as someone else pointed recently).

The simple solution is to remove the bad characters, normalize the case, and then append underscores to disambiguate them, like we currently do for function overloads. That would produce URLs like this, let's say:

/docs/api/faastjs.locationformatter.call_1.md
/docs/api/faastjs.locationformatter.call_2.md
/docs/api/faastjs.locationformatter.call_3.md

The concern with this solution is that it's not stable. In other words, what happens if someone's cleaning up the code and they shuffle around the order of the declarations? The URLs could end up getting mapped differently, which would be very bad for a documentation page with hyperlinks to the old URLs.

TSDoc provides a @label tag that can be used to give nice names to disambiguate items, but api-documenter ideally shouldn't require every declaration to have an @label tag in order to work properly. That wouldn't be a very good experience.

I'm sure this has a simple straightforward solution, I just haven't had much time to think about it. I've been super busy lately. I'll see if I can find some time. If you have any ideas, feel free to propose something! :-P

@Vitalius1 @natalieethell @AlexJerabek FYI

@octogonz octogonz added needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon. labels May 29, 2019
@acchou
Copy link
Contributor Author

acchou commented May 30, 2019

The concern with this solution is that it's not stable. In other words, what happens if someone's cleaning up the code and they shuffle around the order of the declarations? The URLs could end up getting mapped differently, which would be very bad for a documentation page with hyperlinks to the old URLs.

In general, I don't think api-documenter should be excessively worried about stability; a best-effort attempt at it is enough. The reason is that anyone who cares about this problem in practice will need to deal with breaking API changes anyway, therefore documentation versioning is the only way to maintain stable links. Docusaurus already has support for documentation versioning for example: https://docusaurus.io/docs/en/versioning.

On the other hand, mostly-stable is useful in practice, so IMHO it's worth putting some effort into it as long as it doesn't constrain api-documenter development too much.

As for solutions, my preferred solution would be to offer an output mode where classes and other things are output in a single file, thereby mostly eliminating this problem. If that's too drastic, maybe a possible step is to simply merge the declarations that end up having the same name into one file, with different sections. I think this would work well for overloading, for example. In fact, the separate files for each override is confusing and verbose.

If that isn't acceptable, then provide a total ordering to the names where the ordering in the source file is the last resort. This allows other criteria to take precedence, such as putting all normal named functions before special functions, and functions with fewer arguments before functions with more arguments, etc.

@octogonz
Copy link
Collaborator

octogonz commented Jun 2, 2019

The reason is that anyone who cares about this problem in practice will need to deal with breaking API changes anyway, therefore documentation versioning is the only way to maintain stable links.

Suppose our API looks like this:

export declare class Example {
  // /docs/api/example.notallpeople.md
  public notAllPeople: boolean;

  // /docs/api/example.constructor.md
  public constructor(x: number, y: number): Vector;
}

...and then someone inserts new members like this:

export declare class Example {
  // /docs/api/example.notallpeople.md
  public noTallPeople: boolean;

  // /docs/api/example.notallpeople_2.md
  public notAllPeople: boolean;

  // /docs/api/example.constructor.md
  public constructor(serializedVector: string): Vector;

  // /docs/api/example.constructor_2.md
  public constructor(x: number, y: number): Vector;
}

This is not a breaking API change. The developer has merely added some new members. But the URLs get reassigned to completely different meanings.

@octogonz octogonz added the ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete label Jun 2, 2019
@octogonz
Copy link
Collaborator

octogonz commented Jun 2, 2019

This issue belongs to a family of closely related issues. I've created metaissue #1308 to come up with a unified fix that tackles them all together.

@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Feb 26, 2024
@iclanton iclanton moved this from Needs triage to AE/AD in Bug Triage Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon.
Projects
Status: AE/AD
Development

No branches or pull requests

2 participants