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] Optionally include source code coordinates in generated documentation #895

Closed
octogonz opened this issue Oct 22, 2018 · 13 comments · Fixed by #3590
Closed
Labels
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

Comments

@octogonz
Copy link
Collaborator

For open source projects, the documentation web site may need to point people to the source code where an API is declared. API Extractor should track the original source file location in the .api.json files, and optionally render this as part of the API reference.

@ecraig12345
Copy link
Member

Do you have a sense of how hard this would be to implement? In a new project I'm working on, we'd like to have links to source files (don't care about specific line numbers) in the documentation. So if it's not too difficult I'd be willing to help add this feature.

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 1, 2019

@ecraig12345 That would be really great! This is definitely an important feature. Lemme think about the design and follow up...

@octogonz octogonz added 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 labels Nov 4, 2019
@octogonz
Copy link
Collaborator Author

octogonz commented Nov 4, 2019

There are some interesting design considerations for this feature.

Consider this declaration:

node-core-library/src/JsonFile.ts

/**
 * Options for JsonFile.saveJsonFile()
 *
 * @public
 */
export interface IJsonFileSaveOptions extends IJsonFileStringifyOptions {
  /**
   * Creates the folder recursively using FileSystem.ensureFolder()
   * Defaults to false.
   */
  ensureFolderExists?: boolean;
  . . .
}

1. Location units

In the model's tree, the two declaration nodes are IJsonFileSaveOptions and a member ensureFolderExists. When we make a hyperlink to a source file, it consists of a range maybe like this:

  • The source file path: ./src/JsonFile.ts
  • A starting position: (Line 46, Column 18), or Buffer offset 1234
  • An ending position: (Line 46, Column 46), or Buffer offset 1262, or maybe Length 28

Note that these coordinates are interchangeable ONLY if you have access to the original source file. Otherwise we don't know what line number corresponds to String offset 1234. The compiler uses buffer offsets, but maybe Line/Column is better for this scenario. (Or could could store both in the .api.json file.)

2. What to link

There's also a question of what the range should correspond to. Do we link to just the word IJsonFileSaveOptions? Do we link to everything from /** to }? Eventually people will probably ask us for different ranges for different purposes.

(BTW I'm not proposing to implement all this right away. But it can sometimes be helpful to speculate about an "ideal" future design.)

3. Loose linking

Note that API Extractor normally documents exported APIs, which sometimes are different from the original declaration. For example if you do this:

src/index.ts

export { IJsonFileSaveOptions as _IJsonFileSaveOptions } from './src/JsonFile';

...then the API will be called _IJsonFileSaveOptions event though the hyperlinked declaration is going to be called IJsonFileSaveOptions. API Extractor does not consider the export statement as a thing to be documented. The analyzer essentially sees the world like a .d.ts rollup.

Also recall that API Extractor processes .d.ts files, not .ts files. We rely on .d.ts.map files to locate the original source coordinates. These maps are not super accurate. In some cases we may not be able to produce a link at all.

4. Multiple source files

Today each API item is declared in a single source file. But this is likely to change in the future. Certain kinds of TypeScript declaration merging may be treated as a single .api.json declaration. Also microsoft/tsdoc#137 proposes a TSDoc tag @partOf that would allow people to request for separate declarations (possibly from different files) to be modeled as a single documentation item.

5. JSON Verbosity

Source file paths tend to be big, however. If we repeatedly write "sourceFile": "./src/JsonFile.ts" into many different JSON node, it could significantly increase the .api.json file size. Ideally the representation should be concise.

But we also want stable diffs when APIs are updated. For example, if replace the strings with indexes into a central array of source file paths, the indexes might get shuffled whenever a new API is added.

6. Relative paths

Source files will be relative to a project folder. The api-extractor.json config file defines a <projectFolder> token that we could use. But note that it isn't always the same folder as the package.json folder.

Also, although it's a strongly discouraged practice, TypeScript allows people to write import statements that refer to files outside the project folder, even on different disk drives I believe. Just something to think about.

7. Repo URLs

Presumably the main usefulness of these links would be to generate GitHub URLs to the original file. If we're storing relative paths, maybe the ApiPackage can store some optional metadata that can be used to determine the URL. This would avoid encumbering a documentation tool with that work. For example, the @packageDocumentation comment maybe could store a repo URL. Or maybe we could work it out from the package.json repository.url field.

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 4, 2019

Here's the JSON for the IJsonFileSaveOptions snippet from above:

node-core-library.api.json

  {
    "kind": "Interface",
    "canonicalReference": "@microsoft/node-core-library!IJsonFileSaveOptions:interface",
    "docComment": "/**\n * Options for JsonFile.saveJsonFile()\n *\n * @public\n */\n",
    "excerptTokens": [
      {
        "kind": "Content",
        "text": "export interface IJsonFileSaveOptions extends "
      },
      {
        "kind": "Reference",
        "text": "IJsonFileStringifyOptions",
        "canonicalReference": "@microsoft/node-core-library!IJsonFileStringifyOptions:interface"
      },
      {
        "kind": "Content",
        "text": " "
      }
    ],
    "releaseTag": "Public",
    "name": "IJsonFileSaveOptions",
    "members": [
      {
        "kind": "PropertySignature",
        "canonicalReference": "@microsoft/node-core-library!IJsonFileSaveOptions#ensureFolderExists:member",
        "docComment": "/**\n * Creates the folder recursively using FileSystem.ensureFolder() Defaults to false.\n */\n",
        "excerptTokens": [
          {
            "kind": "Content",
            "text": "ensureFolderExists?: "
          },
          {
            "kind": "Content",
            "text": "boolean"
          },
          {
            "kind": "Content",
            "text": ";"
          }
        ],
        "releaseTag": "Public",
        "name": "ensureFolderExists",
        "propertyTypeTokenRange": {
          "startIndex": 1,
          "endIndex": 2
        }
      },
      . . .
    ],
    "extendsTokenRanges": [
      {
        "startIndex": 1,
        "endIndex": 3
      }
    ]
  },

What if we changed the first excerptTokens field like this:

    "excerptTokens": [
      {
        "kind": "Content",
        "text": "export interface "
      },
      { // <== split into a separate token
        "kind": "Content",
        "text": "IJsonFileSaveOptions",
        "sourceFile": "src/JsonFile.ts", // <== new
        "sourceStart": [ 46, 18 ], // <== new
        "sourceEnd": [ 46, 46 ] // <== new
      },
      {
        "kind": "Content",
        "text": " extends "
      },
      {
        "kind": "Reference",
        "text": "IJsonFileStringifyOptions",
        "canonicalReference": "@microsoft/node-core-library!IJsonFileStringifyOptions:interface"
      },
      {
        "kind": "Content",
        "text": " "
      }
    ],

The full-blown design would be like this:

  • "sourceFile": "src/JsonFile.ts" sets up a file path relative to <projectFolder>.
  • (If verbosity is a problem, we could later specify that if "sourceFile" is omitted, the loader should reuse the most recent "sourceFile" that it encountered before.)
  • The "sourceStart": [ 46, 18 ] indicates the [ line, column ] of the sourceFile where the range starts.
  • If we later want to include a buffer index, it can be a third array element like "sourceStart": [ 46, 18, 1234 ]
  • The "sourceEnd": [ 46, 46 ] indicates the non-inclusive end of the range. For an empty range, the sourceEnd would be the same as the sourceStart.
  • The sourceFile, sourceStart, and sourceEnd can be attached to any kind of excerptTokens token. They don't need to all be attached to the same token. Thus a sourceStart / sourceEnd pair can span multiple tokens.
  • I suppose we could also say that sourceEnd can be omitted when it is the same as the next token's sourceStart. In other words, if we encounter a second sourceStart then that automatically ends any previous range.

Then to address the humble scenario of "Give me a GItHub URL for this thing", the ApiDeclaredItem API can expose a new Excerpt property corresponding to some interesting range. Maybe it would be called ApiDeclaredItem.seeSourceExcerpt?

For named things like interface and function I'm thinking we should link to the name identifier (IJsonFileSaveOptions in the above example).

For unnamed things like an indexer ([value: number]: number;) we can just choose something arbitrary like the [ character.

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 4, 2019

So if it's not too difficult I'd be willing to help add this feature.

@ecraig12345 Maybe we can approach it like this: I can start a PR that introduces the new APIs (a minimal subset of the "ideal" proposal above). My PR can also illustrate how to calculate coordinates for a few kinds like class and function. Then you can fill out the implementation for the rest of the API kinds, and test it against your doc app.

(BTW I believe DocFX also supports source-linking. If you're ambitious, maybe later you could come back with a separate PR that updates API Documenter to write these coordinates into the DocFX YAML files. A lot of people would be happy about that.)

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 4, 2019

@rbuckton (who might have some thoughts about these requirements)

@ecraig12345
Copy link
Member

Thanks for thinking this through! Some thoughts in response:

1. Location units

The location/range within the file actually isn't important for my purposes and seems like it would complicate the implementation (especially if you try to go through sourcemaps and link to the original source file rather than the .d.ts), so I wonder if it would be possible to support only paths in the initial implementation and leave within-file ranges for later?

2. What range to link

If there's a way through api-extractor to get the doc comment for an item once you have its name, I think linking to just the name itself might make sense. (I don't have a strong opinion on this though since it's not something we're as likely to use.)

3. Loose linking

This is a good point. To clarify, do you mean we should link to the export line, or the original interface definition (even if the name is different)?

A related tricky case is whole-package re-exports, export * from 'foo'.

6. Relative paths

I think using paths relative to either projectRoot or the package root could be okay, as long as the decision is clearly documented.

7. Repo links

I would be okay with leaving repo link generation to the consumer, at least at first, due to various complicating factors like converting from generated file paths to original source paths (if declaration maps aren't configured), getting the repo URL, and determining the correct branch.

Where to store paths in the JSON

I can see how the proposal of storing the location in excerptTokens makes sense in some ways, but it seems a bit convoluted and adds properties to the excerpt token which would be unset most of the time (and it seems odd to have a token of type Content which is actually a source reference). It also more or less forces ranges within the file to be included in the initial implementation.

I'd suggest something more like this instead (or possibly bundling the info together in an object under a source key). It's generally a more straightforward layout and allows an initial implementation in which only paths are stored.

{
    "kind": "Interface",
    "canonicalReference": "@microsoft/node-core-library!IJsonFileSaveOptions:interface",
    "docComment": "/**\n * Options for JsonFile.saveJsonFile()\n *\n * @public\n */\n",
    "sourceFile": "src/JsonFile.ts", // <== new
    "sourceStart": [ 46, 18 ], // <== new--could add later
    "sourceEnd": [ 46, 46 ], // <== new--could add later

@ecraig12345
Copy link
Member

@octogonz Have you gotten a chance to think about this any more? It's pretty important for the project I'm currently working on. I can probably help with the implementation, but I'll need some examples first (still don't have a good enough mental model of how the TS compiler API works to start this one by myself).

@octogonz
Copy link
Collaborator Author

I haven't been able to look at this yet. I'm out of office right now but will be back after Thanksgiving.

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 25, 2019

I thought about it some more. Seems like I was over-analyzing this problem. You're right that it's probably not super useful to maintain source code coordinates for each excerpt token. It should be enough to track the original source file/line for each API item.

@ecraig12345 If you want to get started on this before I'm back, you can obtain the coordinates for an AstDeclaration like this:

    const astDeclaration: AstDeclaration;
    const sourceFile: ts.SourceFile = astDeclaration.declaration.getSourceFile();
    const sourceFilePath: string = sourceFile.fileName;
    const lineAndCharacter: ts.LineAndCharacter = sourceFile.getLineAndCharacterOfPosition(
      astDeclaration.declaration.pos);

The above code will return the location in the .d.ts file. To translate that to a .ts file path, you'll need to use the source map logic found in SourceMapper.ts.

That's not a whole lot to go on, but maybe you can figure it out. Otherwise I'll put together soon as I get some time. Thanks for your patience!

@vidartf
Copy link
Contributor

vidartf commented Aug 5, 2020

Bumping this for attention (sorry for the noise). Getting the file/line combination would be super useful. E.g. I'm currently making a Github action that warns about changes to the API on a PR. If I had access to the path/line, those warnings could show up directly in the diff view!

@rbuckton
Copy link
Member

FYI: docfx has a specific format for YAML to link to the source code for a documented item: https://dotnet.github.io/docfx/spec/metadata_format_spec.html#23-item-object

image

image

So at the very least, API Extractor might want to keep track of that minimal set of information to be output using api-documenter yaml. Not all source control repos are Git repos however, so you might want to be more flexible. You could also use the "repository" field format in package.json as inspiration as well: https://docs.npmjs.com/files/package.json#repository

@octogonz
Copy link
Collaborator Author

Regarding the DocFX coordinates: In the interests of conciseness, we could split this information into two parts:

Probably the same for all API items within a given NPM package

  • repo URL, for example: https://github.com/microsoft/rushstack.git
  • branch: main
  • revision
  • project folder URL, for example: https://github.com/microsoft/rushstack/tree/main/apps/api-extractor

👆 These can probably be stored centrally with the ApiPackage, rather than duplicating them for every item as implied by the DocFX screenshot above.

Potentially different for each API item

  • Relative path within project folder, for example: src/api/Extractor.ts
  • start line / end line
  • start column / end column (maybe we don't realistically need this?)

👆 Whereas these could be stored separately for each item.

However: the src/api/Extractor.ts string is likely to be the same for many API items declared in the same file. Perhaps the serializer/deserializer could optimize this by only serializing it when it is different from the parent API item.

CC @zelliott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants