Skip to content

Use npm @types/* for 3rd party type definitions #310

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

Merged
merged 6 commits into from
Oct 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
"node": ">=0.10.0"
},
"dependencies": {
"@types/fs-extra": "0.0.33",
"@types/handlebars": "^4.0.31",
"@types/highlight.js": "^9.1.8",
"@types/lodash": "^4.14.37",
"@types/marked": "0.0.28",
"@types/minimatch": "^2.0.29",
"@types/shelljs": "^0.3.32",
"fs-extra": "^0.30.0",
"handlebars": "4.0.5",
"highlight.js": "^9.0.0",
Expand Down
2 changes: 1 addition & 1 deletion src/lib/converter/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ export class Context
return;
}

var isDeclaration = ts.isDeclarationFile(node);
var isDeclaration = node.isDeclarationFile;
if (isDeclaration) {
var lib = this.converter.getDefaultLib();
var isLib = node.fileName.substr(-lib.length) == lib;
Expand Down
3 changes: 2 additions & 1 deletion src/lib/converter/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {Context} from "./context";
import {ConverterComponent, ConverterNodeComponent, ConverterTypeComponent, ITypeTypeConverter, ITypeNodeConverter} from "./components";
import {CompilerHost} from "./utils/compiler-host";
import {Component, Option, ChildableComponent, IComponentClass} from "../utils/component"
import {normalizePath} from "../utils/fs";


/**
Expand Down Expand Up @@ -330,7 +331,7 @@ export class Converter extends ChildableComponent<Application, ConverterComponen
*/
convert(fileNames:string[]):IConverterResult {
for (var i = 0, c = fileNames.length; i < c; i++) {
fileNames[i] = ts.normalizePath(ts.normalizeSlashes(fileNames[i]));
fileNames[i] = normalizePath(ts.normalizeSlashes(fileNames[i]));
}

var program = ts.createProgram(fileNames, this.application.options.getCompilerOptions(), this.compilerHost);
Expand Down
10 changes: 10 additions & 0 deletions src/lib/converter/plugins/GitHubPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ import {BasePath} from "../utils/base-path";
import {Converter} from "../converter";
import {Context} from "../context";

// This should be removed when @typings/shelljs typings are updated to the shelljs version being used
declare module "shelljs" {
// `stdout` was added in:
// https://github.com/shelljs/shelljs/commit/8a7f7ceec4d3a77a9309d935755675ac368b1eda#diff-c3bfabb5e6987aa21bc75ffd95a162d6
// As of 2016-10-16, DefinitelyTyped's defs are for shelljs v0.3.0, but we're on 0.7.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Has anyone created a PR to update this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not find any updates or PRs for shelljs

interface ExecOutputReturnValue {
stdout: string;
stderr: string;
}
}

/**
* Stores data of a repository.
Expand Down
130 changes: 130 additions & 0 deletions src/lib/converter/ts-internal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import * as ts from "typescript";

/**
* Expose the internal TypeScript APIs that are used by TypeDoc
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to identifying the internal APIs so we can move off of them. Way to also link to the internal TS definitions.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any concern about the out-datedness that could occur here? Is there a way to build it with the internal APIs exposed instead and use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any concern about the out-datedness that could occur

That is certainly a concern. My hope is that these internal usages can be replaced by other APIs.

declare module "typescript" {
interface Symbol {
// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2166
id?: number;
Copy link

Choose a reason for hiding this comment

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

we can expose a getter on the Symbol class to expose this as needed. please file an issue for it.

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2168
parent?: ts.Symbol;
Copy link

Choose a reason for hiding this comment

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

We can expose this. please file an issue for it.

}

interface Node {
// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L469
symbol?: ts.Symbol;
Copy link

Choose a reason for hiding this comment

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

use TypeChecker.getSymbolAtLocation instead.

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L472
localSymbol?: ts.Symbol;
Copy link

Choose a reason for hiding this comment

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

do not think you should ever use this. it is misleading, and very subtle.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I used this to get the local name of a default export

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L471
nextContainer?: ts.Node;
Copy link

Choose a reason for hiding this comment

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

do not use this. there are better ways to accomplish what you want.

}

/**
* These functions are in "core" and are marked as @internal:
* https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L4-L5
*/

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L411
// function hasProperty<T>(map: ts.MapLike<T>, key: string): boolean;

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L655-L656
export function createCompilerDiagnostic(message: ts.DiagnosticMessage, ...args: any[]): ts.Diagnostic;
Copy link

Choose a reason for hiding this comment

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

the place this is used in the typedoc code does not look needed. so i would say just do not use it.

export function createCompilerDiagnostic(message: ts.DiagnosticMessage): ts.Diagnostic;

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L701
function compareValues<T>(a: T, b: T): number; // Actually returns a ts.Comparison which is internal
Copy link

Choose a reason for hiding this comment

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

i would say copy this one.


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L790
function normalizeSlashes(path: string): string;
Copy link

Choose a reason for hiding this comment

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

literally:

 export function normalizeSlashes(path: string): string {
        return path.replace(/\\/g, "/");
    }


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L795
function getRootLength(path: string): number;

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L845
// function normalizePath(path: string): string;

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/core.ts#L852-L854
function getDirectoryPath(path: ts.Path): ts.Path;
Copy link

Choose a reason for hiding this comment

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

do not see why not use path functions for node instead. we really do not have this luxury since the compiler can not assume node so we have to re implement some of these helpers.

function getDirectoryPath(path: string): string;
function getDirectoryPath(path: string): any;

/**
* These functions are in "utilities" and are marked as @internal:
* https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L3-L4
*/

// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L188
function getSourceFileOfNode(node: ts.Node): ts.SourceFile;
Copy link

Choose a reason for hiding this comment

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

use Node.getSourceFile instead


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L333
function getTextOfNode(node: ts.Node, includeTrivia?: boolean): string;
Copy link

Choose a reason for hiding this comment

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

use Node.getText instead


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L438
function declarationNameToString(name: ts.DeclarationName);
Copy link

Choose a reason for hiding this comment

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

use TypeChecker.symbolToString instead.


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L598
function getJsDocComments(node: ts.Node, sourceFileOfNode: ts.SourceFile);
Copy link

Choose a reason for hiding this comment

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

use Symbol.getDocumentationComment instead


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L1487
function isBindingPattern(node: ts.Node): node is ts.BindingPattern;
Copy link

Choose a reason for hiding this comment

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

we can expose this. please file an issue.


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L1696
function getClassExtendsHeritageClauseElement(node: ts.ClassLikeDeclaration | ts.InterfaceDeclaration);
Copy link

Choose a reason for hiding this comment

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

do not think this is needed frankly.


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L1701
function getClassImplementsHeritageClauseElements(node: ts.ClassLikeDeclaration);
Copy link

Choose a reason for hiding this comment

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

nor this.


// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L1706
function getInterfaceBaseTypeNodes(node: ts.InterfaceDeclaration);
Copy link

Choose a reason for hiding this comment

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

nor this.



/**
* https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2789-L2924
* This is large enum of char codes.
*
* Faking the enum as a var (only certain codes are used by TypeDoc)
*/
var CharacterCodes: {
Copy link

Choose a reason for hiding this comment

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

we can expose this. though it is just ASCII codes, so you can define them yourself rely.

[key: string]: number;
doubleQuote: number;
space: number;
minus: number;
at: number;
};

/**
* https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2334
* Duplicating the interface definition :(
*/
// interface IntrinsicType extends ts.Type {
// intrinsicName: string;
// }

const optionDeclarations: CommandLineOption[];
Copy link

Choose a reason for hiding this comment

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

not sure why this is needed. but if you have a good use case we can expose it.


/**
* Command line options
*/
interface CommandLineOption {
name: string;
type: string;
shortName: string;
description: DiagnosticsEnumValue;
paramType: DiagnosticsEnumValue;
}

const Diagnostics: {
[key: string]: DiagnosticsEnumValue;
FILE: DiagnosticsEnumValue;
DIRECTORY: DiagnosticsEnumValue;
};

interface DiagnosticsEnumValue {
code: number;
category: ts.DiagnosticCategory;
key: string;
message: string;
}

}
18 changes: 13 additions & 5 deletions src/lib/converter/types/intrinsic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,22 @@ import {Type, IntrinsicType} from "../../models/index";
import {Component, ConverterTypeComponent, ITypeTypeConverter} from "../components";
import {Context} from "../context";

// TypeScript has an @internal enum set for the intrinsic types:
// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2297-L2298
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason you defined this here instead of ts-internal.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in ts-internal.ts exposes the existing internal/hidden APIs by augmenting the "typescript" module and exported interfaces.

In this case, the TypeFlags enum itself is exported from ts namespace. However, the Intrinsic enum value is marked as @hidden. I could not find a way to augment the existing TypeFlags enum (which is exposed as a public API) with a new enum value (which is @internal and hidden). Since I couldn't cleanly expose something from the typescript module using an augmentation, I decided to move it outside that file.

The other option, I suppose, is to export it from the ts-internal.ts file as a separate symbol (not inside the typescript namespace). I don't really have a strong opinion on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. Unless it is needed in multiple places, defining it here is good with me.

Copy link
Member

Choose a reason for hiding this comment

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

What about using any to access this? My only fear is forgetting to update it if anything changes in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with the any approach.

// It is not included in the typescript typings, so the enum is cast as `any` to access the `Intrinsic` set.
const IntrinsicTypeFlags = (ts.TypeFlags as any).Intrinsic;
if (IntrinsicTypeFlags === undefined) {
throw new Error("Internal TypeScript API missing: TypeFlags.Intrinsic");
}

@Component({name:'type:intrinsic'})
export class IntrinsicConverter extends ConverterTypeComponent implements ITypeTypeConverter<ts.IntrinsicType>
export class IntrinsicConverter extends ConverterTypeComponent implements ITypeTypeConverter<ts.Type>
{
/**
* Test whether this converter can handle the given TypeScript type.
*/
supportsType(context:Context, type:ts.IntrinsicType):boolean {
return !!(type.flags & ts.TypeFlags.Intrinsic);
supportsType(context:Context, type:ts.Type):boolean {
return !!(type.flags & IntrinsicTypeFlags);
}


Expand All @@ -28,7 +35,8 @@ export class IntrinsicConverter extends ConverterTypeComponent implements ITypeT
* @param type The intrinsic type that should be converted.
* @returns The type reflection representing the given intrinsic type.
*/
convertType(context:Context, type:ts.IntrinsicType):IntrinsicType {
return new IntrinsicType(type.intrinsicName);
convertType(context:Context, type:ts.Type):IntrinsicType {
let intrinsicName = context.program.getTypeChecker().typeToString(type);
return new IntrinsicType(intrinsicName);
}
}
3 changes: 2 additions & 1 deletion src/lib/converter/utils/compiler-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as ts from "typescript";
import * as Path from "path";

import {ConverterComponent} from "../components";
import {normalizePath} from "../../utils/fs";


/**
Expand Down Expand Up @@ -56,7 +57,7 @@ export class CompilerHost extends ConverterComponent implements ts.CompilerHost
*/
getDefaultLibFileName(options:ts.CompilerOptions):string {
var lib = this.owner.getDefaultLib();
var path = ts.getDirectoryPath(ts.normalizePath(require.resolve('typescript')));
var path = ts.getDirectoryPath(normalizePath(require.resolve('typescript')));
return Path.join(path, lib);
}

Expand Down
6 changes: 3 additions & 3 deletions src/lib/utils/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var existingDirectories:ts.MapLike<boolean> = {};
* @returns The normalized path.
*/
export function normalizePath(path:string) {
return ts.normalizePath(path);
return path.replace(/\\/g, "/");
}


Expand All @@ -26,7 +26,7 @@ export function normalizePath(path:string) {
* @returns TRUE if the given directory exists, FALSE otherwise.
*/
export function directoryExists(directoryPath: string): boolean {
if (ts.hasProperty(existingDirectories, directoryPath)) {
if (existingDirectories.hasOwnProperty(directoryPath)) {
return true;
}

Expand Down Expand Up @@ -65,7 +65,7 @@ export function ensureDirectoriesExist(directoryPath: string) {
*/
export function writeFile(fileName:string, data:string, writeByteOrderMark:boolean, onError?:(message:string) => void) {
try {
ensureDirectoriesExist(ts.getDirectoryPath(ts.normalizePath(fileName)));
ensureDirectoriesExist(ts.getDirectoryPath(normalizePath(fileName)));
ts.sys.writeFile(fileName, data, writeByteOrderMark);
} catch (e) {
if (onError) onError(e.message);
Expand Down
Loading