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] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

Merged
merged 11 commits into from
Nov 28, 2018
10 changes: 3 additions & 7 deletions apps/api-documenter/src/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@

import * as os from 'os';
import * as colors from 'colors';
import * as path from 'path';

import { FileConstants } from '@microsoft/node-core-library';
import { PackageJsonLookup } from '@microsoft/node-core-library';

import { ApiDocumenterCommandLine } from './cli/ApiDocumenterCommandLine';

const myPackageJsonFilename: string = path.resolve(path.join(
__dirname, '..', FileConstants.PackageJson)
);
const myPackageJson: { version: string } = require(myPackageJsonFilename);
const myPackageVersion: string = PackageJsonLookup.loadOwnPackageJson(__dirname, '..').version;
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

Do you actually need the ..? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, package.json is above the src folder.

We could walk upwards to probe for package.json, but that's inefficient and error-prone for a scenario that is supposed to be basically hardcoded.


In reply to: 236574942 [](ancestors = 236574942)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I think you persuaded me


In reply to: 236587957 [](ancestors = 236587957,236574942)


console.log(os.EOL + colors.bold(`api-documenter ${myPackageJson.version} `
console.log(os.EOL + colors.bold(`api-documenter ${myPackageVersion} `
+ colors.cyan(' - http://aka.ms/extractor') + os.EOL));

const parser: ApiDocumenterCommandLine = new ApiDocumenterCommandLine();
Expand Down
14 changes: 8 additions & 6 deletions apps/api-extractor/src/ExtractorContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export interface IExtractorContextOptions {
* abstract syntax tree.
*/
export class ExtractorContext {
public typeChecker: ts.TypeChecker;
public readonly program: ts.Program;
public readonly typeChecker: ts.TypeChecker;
public package: AstPackage;

/**
Expand All @@ -73,12 +74,12 @@ export class ExtractorContext {

public readonly validationRules: IExtractorValidationRulesConfig;

public readonly logger: ILogger;

// If the entry point is "C:\Folder\project\src\index.ts" and the nearest package.json
// is "C:\Folder\project\package.json", then the packageFolder is "C:\Folder\project"
private _packageFolder: string;

private _logger: ILogger;

constructor(options: IExtractorContextOptions) {
this.packageJsonLookup = new PackageJsonLookup();

Expand All @@ -97,7 +98,7 @@ export class ExtractorContext {

this.docItemLoader = new DocItemLoader(this._packageFolder);

this._logger = options.logger;
this.logger = options.logger;

// This runs a full type analysis, and then augments the Abstract Syntax Tree (i.e. declarations)
// with semantic information (i.e. symbols). The "diagnostics" are a subset of the everyday
Expand All @@ -107,6 +108,7 @@ export class ExtractorContext {
this.reportError(`TypeScript: ${errorText}`, diagnostic.file, diagnostic.start);
}

this.program = options.program;
this.typeChecker = options.program.getTypeChecker();

const rootFile: ts.SourceFile | undefined = options.program.getSourceFile(options.entryPointFile);
Expand Down Expand Up @@ -146,10 +148,10 @@ export class ExtractorContext {

// Format the error so that VS Code can follow it. For example:
// "src\MyClass.ts(15,1): The JSDoc tag "@blah" is not supported by AEDoc"
this._logger.logError(`${shownPath}(${lineAndCharacter.line + 1},${lineAndCharacter.character + 1}): `
this.logger.logError(`${shownPath}(${lineAndCharacter.line + 1},${lineAndCharacter.character + 1}): `
+ message);
} else {
this._logger.logError(message);
this.logger.logError(message);
}
}

Expand Down
16 changes: 14 additions & 2 deletions apps/api-extractor/src/extractor/Extractor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import {
JsonFile,
JsonSchema,
Path,
FileSystem
FileSystem,
PackageJsonLookup
} from '@microsoft/node-core-library';
import {
IExtractorConfig,
Expand All @@ -25,6 +26,7 @@ import { ApiFileGenerator } from '../generators/ApiFileGenerator';
import { DtsRollupGenerator, DtsRollupKind } from '../generators/dtsRollup/DtsRollupGenerator';
import { MonitoredLogger } from './MonitoredLogger';
import { TypeScriptMessageFormatter } from '../utils/TypeScriptMessageFormatter';
import { PackageMetadataManager } from '../generators/dtsRollup/PackageMetadataManager';

/**
* Options for {@link Extractor.processProject}.
Expand Down Expand Up @@ -119,6 +121,13 @@ export class Extractor {
private readonly _monitoredLogger: MonitoredLogger;
private readonly _absoluteRootFolder: string;

/**
* The NPM package version for the currently executing instance of the "\@microsoft/api-extractor" library.
*/
public static get version(): string {
return PackageJsonLookup.loadOwnPackageJson(__dirname, '../..').version;
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

Same here. #ByDesign

}

/**
* Given a list of absolute file paths, return a list containing only the declaration
* files. Duplicates are also eliminated.
Expand Down Expand Up @@ -399,6 +408,9 @@ export class Extractor {

this._generateRollupDtsFiles(context);

// Write the tsdoc-metadata.json file for this project
PackageMetadataManager.writeTsdocMetadataFile(context.packageFolder);

if (this._localBuild) {
// For a local build, fail if there were errors (but ignore warnings)
return this._monitoredLogger.errorCount === 0;
Expand Down Expand Up @@ -486,7 +498,7 @@ export class Extractor {
this._monitoredLogger.logVerbose(`Writing package typings: ${mainDtsRollupFullPath}`);

dtsRollupGenerator.writeTypingsFile(mainDtsRollupFullPath, dtsKind);
}
}

private _getShortFilePath(absolutePath: string): string {
if (!path.isAbsolute(absolutePath)) {
Expand Down
39 changes: 24 additions & 15 deletions apps/api-extractor/src/generators/dtsRollup/AstSymbolTable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { AstSymbol } from './AstSymbol';
import { AstImport } from './AstImport';
import { AstEntryPoint, IExportedMember } from './AstEntryPoint';
import { PackageMetadataManager } from './PackageMetadataManager';
import { ILogger } from '../../extractor/ILogger';

/**
* AstSymbolTable is the workhorse that builds AstSymbol and AstDeclaration objects.
Expand All @@ -22,6 +23,7 @@ import { PackageMetadataManager } from './PackageMetadataManager';
* is "exported" or not.)
*/
export class AstSymbolTable {
private readonly _program: ts.Program;
private readonly _typeChecker: ts.TypeChecker;
private readonly _packageJsonLookup: PackageJsonLookup;
private readonly _packageMetadataManager: PackageMetadataManager;
Expand Down Expand Up @@ -54,10 +56,13 @@ export class AstSymbolTable {
private readonly _astEntryPointsBySourceFile: Map<ts.SourceFile, AstEntryPoint>
= new Map<ts.SourceFile, AstEntryPoint>();

public constructor(typeChecker: ts.TypeChecker, packageJsonLookup: PackageJsonLookup) {
public constructor(program: ts.Program, typeChecker: ts.TypeChecker, packageJsonLookup: PackageJsonLookup,
logger: ILogger) {

this._program = program;
this._typeChecker = typeChecker;
this._packageJsonLookup = packageJsonLookup;
this._packageMetadataManager = new PackageMetadataManager(packageJsonLookup);
this._packageMetadataManager = new PackageMetadataManager(packageJsonLookup, logger);
}

/**
Expand All @@ -73,16 +78,18 @@ export class AstSymbolTable {
throw new Error('Unable to find a root declaration for ' + sourceFile.fileName);
}

if (!this._packageMetadataManager.isAedocSupportedFor(sourceFile.fileName)) {
const packageJsonPath: string | undefined = this._packageJsonLookup
.tryGetPackageJsonFilePathFor(sourceFile.fileName);

if (packageJsonPath) {
throw new Error(`Please add a field such as "tsdoc": { "tsdocFlavor": "AEDoc" } to this file:\n`
+ packageJsonPath);
} else {
throw new Error(`The specified entry point does not appear to have an associated package.json file:\n`
+ sourceFile.fileName);
if (this._program.isSourceFileFromExternalLibrary(sourceFile)) {
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

Is this right, or is this condition backwards? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's right. If the source file is NOT from an external library, then it's part of the project we're analyzing, which obviously supports API Extractor.


In reply to: 236575683 [](ancestors = 236575683)

if (!this._packageMetadataManager.isAedocSupportedFor(sourceFile.fileName)) {
const packageJsonPath: string | undefined = this._packageJsonLookup
.tryGetPackageJsonFilePathFor(sourceFile.fileName);

if (packageJsonPath) {
throw new Error(`Please add a field such as "tsdoc": { "tsdocFlavor": "AEDoc" } to this file:\n`
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

Didn't we say we wanted to just use the metadata file if we're generating a rollup? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch -- I meant to delete this code


In reply to: 236579005 [](ancestors = 236579005)

+ packageJsonPath);
} else {
throw new Error(`The specified entry point does not appear to have an associated package.json file:\n`
+ sourceFile.fileName);
}
}
}

Expand Down Expand Up @@ -314,9 +321,11 @@ export class AstSymbolTable {

// If the file is from a package that does not support AEDoc, then we process the
// symbol itself, but we don't attempt to process any parent/children of it.
if (!this._packageMetadataManager.isAedocSupportedFor(
followedSymbol.declarations[0].getSourceFile().fileName)) {
nominal = true;
const followedSymbolSourceFile: ts.SourceFile = followedSymbol.declarations[0].getSourceFile();
if (this._program.isSourceFileFromExternalLibrary(followedSymbolSourceFile)) {
if (!this._packageMetadataManager.isAedocSupportedFor(followedSymbolSourceFile.fileName)) {
nominal = true;
}
}

let parentAstSymbol: AstSymbol | undefined = undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ export class DtsRollupGenerator {
this._context = context;
this._typeChecker = context.typeChecker;
this._tsdocParser = new tsdoc.TSDocParser();
this._astSymbolTable = new AstSymbolTable(this._context.typeChecker, this._context.packageJsonLookup);
this._astSymbolTable = new AstSymbolTable(this._context.program, this._context.typeChecker,
this._context.packageJsonLookup, context.logger);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

import * as path from 'path';

import {
PackageJsonLookup,
IPackageJson
IPackageJson,
FileSystem,
JsonFile,
NewlineKind
} from '@microsoft/node-core-library';
import { Extractor } from '../../extractor/Extractor';
import { ILogger } from '../../extractor/ILogger';

/**
* Represents analyzed information for a package.json file.
Expand All @@ -26,37 +33,64 @@ export class PackageMetadata {
*/
public readonly aedocSupported: boolean;

private readonly _packageJsonLookup: PackageJsonLookup;

public constructor(packageJsonPath: string, packageJsonLookup: PackageJsonLookup) {
this._packageJsonLookup = packageJsonLookup;
public constructor(packageJsonPath: string, packageJson: IPackageJson, aedocSupported: boolean) {
this.packageJsonPath = packageJsonPath;

this.packageJson = this._packageJsonLookup.loadPackageJson(packageJsonPath);

this.aedocSupported = false;

if (this.packageJson.tsdoc) {
if (this.packageJson.tsdoc.tsdocFlavor) {
if (this.packageJson.tsdoc.tsdocFlavor.toUpperCase() === 'AEDOC') {
this.aedocSupported = true;
}
}
}
this.packageJson = packageJson;
this.aedocSupported = aedocSupported;
}
}

/**
* This class maintains a cache of analyzed information obtained from package.json
* files. It is built on top of the PackageJsonLookup class.
*
* @remarks
*
* IMPORTANT: Don't use PackageMetadataManager to analyze source files from the current project:
* 1. Files such as tsdoc-metadata.json may not have been built yet, and thus may contain incorrect information.
* 2. The current project is not guaranteed to have a package.json file at all. For example, API Extractor can
* be invoked on a bare .d.ts file.
*
* Use ts.program.isSourceFileFromExternalLibrary() to test source files before passing the to PackageMetadataManager.
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

What would the perf impact be if we tested for this in PackageMetadataManager? #WontFix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally if you're inside PackageMetadataManager for a local file, it's already too late.

I could add an assertion here to catch that mistake however.

PackageMetadataManager is a fairly half-baked file right now, though. I want to make tsdoc-metadata.json a feature of the TSDoc library, and then we'll redesign PackageMetadataManager when we update it to use that.


In reply to: 236576224 [](ancestors = 236576224)

*/
export class PackageMetadataManager {
public static tsdocMetadataFilename: string = 'tsdoc-metadata.json';

private readonly _packageJsonLookup: PackageJsonLookup;
private readonly _logger: ILogger;
private readonly _packageMetadataByPackageJsonPath: Map<string, PackageMetadata>
= new Map<string, PackageMetadata>();

public constructor(packageJsonLookup: PackageJsonLookup) {
public static writeTsdocMetadataFile(packageJsonFolder: string): void {
// This feature is still being standardized: https://github.com/Microsoft/tsdoc/issues/7
// In the future we will use the @microsoft/tsdoc library to read this file.
const tsdocMetadataPath: string = path.join(packageJsonFolder,
'dist', PackageMetadataManager.tsdocMetadataFilename);
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

dist is an arbitrary assumption to make. This should probably be placed in the same directory as the rollup file. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rollup file may not exist. But we could put it in the same directory as the default entry point for the NPM package.


In reply to: 236576554 [](ancestors = 236576554)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated TSDoc #7 to reflect this idea.

However I'm not going to implement it here, since this is a temporary workaround.


In reply to: 236597139 [](ancestors = 236597139,236576554)


const fileObject: Object = {
tsdocVersion: '0.12',
toolPackages: [
{
packageName: '@microsoft/api-extractor',
packageVersion: Extractor.version
}
]
};

const fileContent: string =
'// This file is read by tools that parse documentation comments conforming to the TSDoc standard.\n'
+ '// It should be published with your NPM package. It should not be tracked by Git.\n'
+ JsonFile.stringify(fileObject);

FileSystem.writeFile(tsdocMetadataPath, fileContent, {
convertLineEndings: NewlineKind.CrLf,
ensureFolderExists: true
});
}

public constructor(packageJsonLookup: PackageJsonLookup, logger: ILogger) {
this._packageJsonLookup = packageJsonLookup;
this._logger = logger;
}

/**
Expand All @@ -72,16 +106,34 @@ export class PackageMetadataManager {
}
let packageMetadata: PackageMetadata | undefined
= this._packageMetadataByPackageJsonPath.get(packageJsonFilePath);

if (!packageMetadata) {
packageMetadata = new PackageMetadata(packageJsonFilePath, this._packageJsonLookup);
const packageJson: IPackageJson = this._packageJsonLookup.loadPackageJson(packageJsonFilePath);

const packageJsonFolder: string = path.dirname(packageJsonFilePath);

// This feature is still being standardized: https://github.com/Microsoft/tsdoc/issues/7
// In the future we will use the @microsoft/tsdoc library to read this file.
let aedocSupported: boolean = false;

const tsdocMetadataPath: string = path.join(packageJsonFolder,
'dist', PackageMetadataManager.tsdocMetadataFilename);

if (FileSystem.exists(tsdocMetadataPath)) {
this._logger.logVerbose('Found metadata in ' + tsdocMetadataPath);
// If the file exists at all, assume it was written by API Extractor
aedocSupported = true;
}

packageMetadata = new PackageMetadata(packageJsonFilePath, packageJson, aedocSupported);
this._packageMetadataByPackageJsonPath.set(packageJsonFilePath, packageMetadata);
}

return packageMetadata;
}

/**
* Returns true if the source file has an associated PackageMetadata object
* with aedocSupported=true.
* Returns true if the source file is part of a package whose .d.ts files support AEDoc annotations.
*/
public isAedocSupportedFor(sourceFilePath: string): boolean {
const packageMetadata: PackageMetadata | undefined = this.tryFetchPackageMetadata(sourceFilePath);
Expand Down
10 changes: 3 additions & 7 deletions apps/api-extractor/src/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@

import * as os from 'os';
import * as colors from 'colors';
import * as path from 'path';

import { FileConstants } from '@microsoft/node-core-library';
import { PackageJsonLookup } from '@microsoft/node-core-library';

import { ApiExtractorCommandLine } from './cli/ApiExtractorCommandLine';

const myPackageJsonFilename: string = path.resolve(path.join(
__dirname, '..', FileConstants.PackageJson)
);
const myPackageJson: { version: string } = require(myPackageJsonFilename);
const myPackageVersion: string = PackageJsonLookup.loadOwnPackageJson(__dirname, '..').version;
Copy link
Member

@iclanton iclanton Nov 27, 2018

Choose a reason for hiding this comment

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

This should just use Extractor.version #Resolved


console.log(os.EOL + colors.bold(`api-extractor ${myPackageJson.version} `
console.log(os.EOL + colors.bold(`api-extractor ${myPackageVersion} `
+ colors.cyan(' - http://aka.ms/extractor') + os.EOL));

const parser: ApiExtractorCommandLine = new ApiExtractorCommandLine();
Expand Down
Loading