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

christopherthielen · 2016-10-16T17:38:39Z

This PR removes the typings files from typedoc/src/typings in favor of using npm @types/* packages.

It now (automatically) uses the official typescript.d.ts definitions from node_modules/typescript/lib/typescript.d.ts.

TypeDoc uses quite a few internal TypeScript APIs. Those APIs were exposed in the (old?) typescript.d.ts from DefinitelyTyped.
To accommodate TypeDoc's existing use of internal TypeScript APIs, I created a ts-internal.ts file. The file augments the official type definitions with the internal APIs that we are using.
Obviously it would be best not to use the internal APIs at all. This first pass can be used to identify which internal APIs we are using, so we can work towards replacing those uses with official API.

- Use the latest `typescript.d.ts` definitions - Add `ts-internal.ts` to expose the internal Typescript APIs (that TypeDoc is currently using) using declaration merging - Remove tsd and typings files Closes TypeStrong#309

aciccarello

I like all these changes, especially pulling out the internal TS apis that are used. Glad to see people helping move this forward 👍

aciccarello · 2016-10-17T13:41:33Z

src/lib/converter/plugins/GitHubPlugin.ts

+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


Has anyone created a PR to update this yet?

I did not find any updates or PRs for shelljs

aciccarello · 2016-10-17T13:42:20Z

src/lib/converter/ts-internal.ts

+
+/**
+ * Expose the internal TypeScript APIs that are used by TypeDoc
+ */


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

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?

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.

aciccarello · 2016-10-17T13:44:46Z

src/lib/converter/types/intrinsic.ts

@@ -4,6 +4,20 @@ import {Type, IntrinsicType} from "../../models/index";
 import {Component, ConverterTypeComponent, ITypeTypeConverter} from "../components";
 import {Context} from "../context";

+// Copy typescript's @internal enum set from:
+// https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2297-L2298


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

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.

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

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

I'm ok with the any approach.

aciccarello · 2016-10-17T13:47:39Z

tsconfig.json

@@ -13,32 +13,19 @@
        "experimentalDecorators": true
    },
    "exclude": [
-        "./node_modules/typescript/lib/typescript.d.ts",
-        "./typescript/lib/typescript.d.ts"
    ],
    "filesGlob": [


Should files/filesGlob be migrated to include?

I'm not sure. What do you think? I didn't change it for fear of "not knowing what I don't know".

I honestly haven't used include yet but I think filesGlob is only supported via plugins so it would make sense to me to move to the official property with glob support. I'm curious what others think.

Kill filesGlob, nothing uses it anyway.

will kill filesGlob and the now empty exclude

blakeembrey · 2016-10-18T00:35:57Z

Thanks a ton! It's really awesome. My only concern is with the possible out-datedness that can occur when we pull the TypeScript definition in manually, but it's an amazing start. Thanks for the PR 😄

aciccarello · 2016-10-18T00:42:38Z

My only concern is with the possible out-datedness that can occur when we pull the TypeScript definition in manually, but it's an amazing start.

@christopherthielen Did you only bring in the definitions that were used? It'd be great to try to reduce this list with future PRs (anyone interested 😉).

christopherthielen · 2016-10-18T00:44:08Z

@aciccarello yes, the only internal APIs I added typings for are the APIs that TypeDoc is currently using.

christopherthielen · 2016-10-18T01:20:06Z

My only concern is with the possible out-datedness that can occur when we pull the TypeScript definition in manually

When I created this PR, I thought the typescript.d.ts that was committed came from DefinitelyTyped. However, I now see that the definitions committed a few days ago is recent (2.0.3) and does include the private APIs. Maybe @DatenMetzgerX can explain where the definitions came from?

I did a little experimenting and found out that it's trivial to generate the full typescript.d.ts including the @internal APIs:

git clone https://github.com/Microsoft/TypeScript.git
cd TypeScript
npm i
perl -pi.bak -e 's/stripInternal: true/stripInternal: false/g' Jakefile.js
npm run build:compiler
cp built/local/typescript.d.ts ../typedoc/typescript.d.ts

I'll see if I can automate the generation of a subset of that file, including only the internal apis being used.

Potentially we could just use the full typescript.d.ts with ALL the internal APIs exposed, but I'd much prefer to limit the visibility to those APIs.

blakeembrey · 2016-10-18T01:30:33Z

Understandable. FWIW, I think ntypescript exposes all internal APIs. You could look at how that is generated. /cc @basarat

MichaReiser · 2016-10-18T05:18:31Z

@christopherthielen

It's generated as you have described. There is an UPDATING.md that describes the updating process in detail

christopherthielen · 2016-10-18T05:37:56Z

@DatenMetzgerX oh, that's great to know and understand, thanks! I completely missed those instructions.

I guess the question related to this PR is then: which would ya'll prefer:

1. continue exposing the entire internal typescript API
1. only expose the internal APIs actually used

Option 1 is the easiest, and is what the project currently uses. We could move to @types/ for only the third party libs and use the UPDATING.md process typescript.d.ts with all internal apis exposed.

Option 2 could be automated and might be a step towards a goal of eliminating (or reducing) use of internal APIs altogether. I have this process partially automated. I suppose I'll finish automating it and add it to the PR, then ya'll can take a look and make a decision.

felixfbecker · 2016-10-18T13:23:22Z

The only question I have: will this cause "duplicate identifier" errors, if you have some of the typings already installed?

christopherthielen · 2016-10-18T15:44:51Z

@felixfbecker this is only for building typedoc, so it shouldn't affect your project's doc generation.

felixfbecker · 2016-10-18T17:08:00Z

@christopherthielen I don't think so, when using typings programmatically in TypeScript it will depend on the typings.

blakeembrey · 2016-10-18T17:54:48Z

@felixfbecker That's a real PITA. I remember having this conversation with the TypeScript team before because we can fix this on our end by providing a custom typeRoots, but I don't think typeRoots is part of the compilation output which means our configuration means nothing when we publish. /cc @mhegazy @DanielRosenwasser what's the correct approach we should take here and does/will the TypeScript definition output be updated to include the typeRoot used with the references?

christopherthielen · 2016-10-18T18:38:51Z

@felixfbecker @blakeembrey I'm not sure I understand what the problem is. Can one of you clarify?

Is the problem related to using @types/ in general?

Is it about using the internal TypeScript APIs?

Or about exposing those internal APIs in ts-internal.ts as part of the TypeDoc build?

blakeembrey · 2016-10-18T19:13:56Z

@christopherthielen Shouldn't affect the existing discussion, but in case you're interested it's related to #281. Basically, we should move to a system where we can publish the typings included so people can use it programmatically. However, if we declare module it'll either cause conflicts with the official TypeScript definition in a consumers project, I believe, or not be used. Either way, probably not what we want. The workaround for this in TS 2.0 is to write external module definitions (a la node_modules/@types). However, we can't do this because it's confusing and not supported - so a workaround is typeRoots in 2.0. However, this feature won't work either because the .d.ts file doesn't emit the typeRoots for a /// <reference types="typescript" /> dependency so even though we'd use it, and publish it, anyone consuming the project will fail.

For now, don't worry about it, it's just a related conversation that was somewhat relevant and it's a good example for a point I logged a while back in May (?) for TS 2.0 module support.

Edit: Note that this is only a solid issue if we expose any transitive types. We might not even do that, which makes the issue semi-mute as the workaround is just avoid it. However, it's still something that may occur unexpectedly in the future if we ever accidentally re-expose a type. A bigger issue too is that the definition we re-expose could end up different to the one we actually use (if it's compiled separately) which would be bad because the module we publish can't even be used with TypeScript.

christopherthielen · 2016-10-18T19:38:39Z

@blakeembrey based on this information, I think we should do the following:

Stop referencing internal API functions from the "typescript" module.

Instead of adding them back into the "typescript" module (like ts-internal currently does) we should simply export them from ts-internal.ts and import from there where used in the TypeDoc code.

import { getJsDocComments } from "../ts-internal";

Augmenting an exported TypeScript interface should be fine. Duplicated interface merges are handled nicely by TypeScript.

For example, this works:

declare module "typescript" {
  interface Symbol {
    id?: number;
    parent?: ts.Symbol;
  }
}

declare module "typescript" {
  interface Symbol {
    parent?: ts.Symbol;
  }
}

Re-declare and export the (completely hidden) @internal enum and interfaces from ts-internal

export interface CommandLineOption {
    name: string;
    type: string;
    shortName: string;
    description: DiagnosticsEnumValue;
    paramType: DiagnosticsEnumValue;
  }

Of course, we'll have to maintain them ourselves, but it seems like this approach would address both issues.

christopherthielen · 2016-10-18T19:40:27Z

For what it's worth, I'm also affected by #281. My kludge currently looks like so: https://github.com/christopherthielen/typedoc-plugin-internal-external/blob/master/tsconfig.json#L12

felixfbecker · 2016-10-18T20:19:11Z

@christopherthielen What about the handlebars typings dependency? Currently it gives a "cannot find module..." when using typedoc programmatically. I assume with this PR that will go away, but what if I already have typings for handlebars installed in my project, e.g. some file that does declare module 'handlebars'? That was my question: would this PR then cause a "duplicate identifier ..." error?

christopherthielen · 2016-10-18T20:27:42Z

@felixfbecker because the typings in this PR are listed as devDependencies, I don't think a project which depends on typedoc would be affected.

blakeembrey · 2016-10-18T20:29:35Z

@christopherthielen That's a good point. Given that, can we make them dependencies? It'll be much more valuable if someone can actually use this module from NPM without having to manually install the dependent types.

mhegazy · 2016-10-18T20:31:05Z

@felixfbecker That's a real PITA. I remember having this conversation with the TypeScript team before because we can fix this on our end by providing a custom typeRoots, but I don't think typeRoots is part of the compilation output which means our configuration means nothing when we publish. /cc @mhegazy @DanielRosenwasser what's the correct approach we should take here and does/will the TypeScript definition output be updated to include the typeRoot used with the references?

I would say the core here is not custom typeRoots, but the use of private APIs. These APIs are marked private as there are no support promises here. so they could be broken at will or removed all together.

As for typeRoots; this is mainly for users who want to override declarations of a certain package, but I do not think it should be used to publish a dependency. republishing declarations of other packages in general have proved painful for everybody. so my recommendation is to get the original package author to include them (in this case Typescript :)), or have a new package that "expands" on the original package, e.g. publish blah-internal that is not blah and then use that instead.

I left a few comments on the PR. Feel free to ignore them, i just found it easier to leave them on the code rather than listing them all on one comment.

mhegazy · 2016-10-18T19:49:16Z

src/lib/converter/ts-internal.ts

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


use TypeChecker.getSymbolAtLocation instead.

mhegazy · 2016-10-18T19:49:36Z

src/lib/converter/ts-internal.ts

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


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

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

mhegazy · 2016-10-18T19:50:00Z

src/lib/converter/ts-internal.ts

+    // https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2166
+    id?: number;
+    // https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L2168
+    parent?: ts.Symbol;


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

mhegazy · 2016-10-18T19:50:24Z

src/lib/converter/ts-internal.ts

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


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

mhegazy · 2016-10-18T19:50:52Z

src/lib/converter/ts-internal.ts

+    // https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L472
+    localSymbol?: ts.Symbol;
+    // https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/types.ts#L471
+    nextContainer?: ts.Node;


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

mhegazy · 2016-10-18T19:57:25Z

src/lib/converter/ts-internal.ts

+   *
+   * Faking the enum as a var (only certain codes are used by TypeDoc)
+   */
+  var CharacterCodes: {


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

mhegazy · 2016-10-18T19:57:47Z

src/lib/converter/ts-internal.ts

+   * Duplicating the interface definition :(
+   */
+  interface IntrinsicType extends ts.Type {
+    intrinsicName: string;


use TypeChecker.typeToString(type) instead.

mhegazy · 2016-10-18T19:58:21Z

src/lib/converter/ts-internal.ts

+    intrinsicName: string;
+  }
+
+  const optionDeclarations: CommandLineOption[];


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

mhegazy · 2016-10-18T19:59:25Z

src/lib/converter/ts-internal.ts

+  function getTextOfNode(node: ts.Node, includeTrivia?: boolean): string;
+
+  // https://github.com/Microsoft/TypeScript/blob/v2.0.5/src/compiler/utilities.ts#L438
+  function declarationNameToString(name: ts.DeclarationName);


use TypeChecker.symbolToString instead.

mhegazy · 2016-10-18T19:59:41Z

src/lib/converter/ts-internal.ts

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


felixfbecker · 2016-10-18T20:33:42Z

@mhegazy What do you think about the fact that this module depends on some packages that do not have included typings, like handlebars? The idea is to make @types/handlebars a dependency so that a user can use typedoc programmatically without getting "cannot find module..." errors.

christopherthielen · 2016-10-18T20:35:12Z

@mhegazy thanks for the feedback on the internal API usage!

christopherthielen · 2016-10-18T20:40:50Z

I'll make another attempt at this PR in the next few days taking into account all this new information.

mhegazy · 2016-10-18T20:44:30Z

@mhegazy What do you think about the fact that this module depends on some packages that do not have included typings, like handlebars? The idea is to make @types/handlebars a dependency so that a user can use typedoc programmatically without getting "cannot find module..." errors.

Let's say typedoc has a dependency with @types/handlebars (after this PR), now if you depend on typedoc and you have another declaratio for handlebars (say in typeings/handlebars/index.d.ts) you will get a duplicate declaration error.

The issue here is really how handelbars is authored, more on this later. but let's say you can not change @types\handelbars. so the recommendation to override it is to add you typings folder in the typeRoots. this way you are using your local version of the definition. e.g.:

"compilerOptions": {
    "typeRoots": [ "./typings", "./node_modules/@types" ]
}

this way the compiler will look at ./typings/handelbars/index.d.ts before looking at /node_modules/@types/handelbars/index.d.t.s.

But the main problem here is https://github.com/DefinitelyTyped/DefinitelyTyped/blob/types-2.0/handlebars/index.d.ts, it should be authored as:

...
export = Handlebars;

instead of:

...
declare module "handlebars" {
    export = Handlebars;
}

…eToString()

blakeembrey · 2016-10-19T23:02:05Z

package.json

+    "@types/lodash": "^4.14.37",
+    "@types/marked": "0.0.28",
+    "@types/minimatch": "^2.0.29",
+    "@types/shelljs": "^0.3.32",


If we can move all these to dependencies, I'd be happy to merge today and leave the rest of the awesome comments from @mhegazy for a future PR. I'll open an issue for them too, once this PR lands.

done.

I'm not 100% sure this is the right thing to do though. I think we only need to add as dependencies those @types/* packages which are exposed via a public API in the typedoc.d.ts files.

@christopherthielen No, user's code won't compile if not all modules referenced in the declaration files are available.

@felixfbecker I guess that's my point, though. Only those typings that are referenced in the declaration files need to be in dependencies.

Edit: in fact, only @types/handlebars is referenced in the typedoc declaration files:

╰─ git:[masterUNSTAGED]  grep -R "///" . ./output/events.d.ts:/// <reference types="handlebars" /> ./output/utils/resources/templates.d.ts:/// <reference types="handlebars" />

That's understandable. The issue with it is that you might accidentally expose one or the other at some point and forget to move them around. I think putting them in the same place as the real dependency is the best approach to avoid unfortunate side-effects.

blakeembrey · 2016-10-20T00:14:59Z

@mhegazy Thanks for all your comments.

I would say the core here is not custom typeRoots, but the use of private APIs. These APIs are marked private as there are no support promises here. so they could be broken at will or removed all together.

I agree, I was trying to allude to the larger problem as it was part of a discussion we'd had in the past. As a module author, if there's an incorrect definition, I'd like to override typeRoots for development and probably publish. I don't want to have to wait days or months just to release a working version of software. This is unrelated to this module, but the bigger issue of typeRoots as a temporary solution. For reference, I've already run into this situation in production but because I use Typings and not TypeScript @types yet I had no trouble working around it (Typings can install from GitHub URLs, I just installed my fork and the PR is still waiting).

Fetch 3rd party type definitions from npm @types/*

52a4956

- Use the latest `typescript.d.ts` definitions - Add `ts-internal.ts` to expose the internal Typescript APIs (that TypeDoc is currently using) using declaration merging - Remove tsd and typings files Closes TypeStrong#309

aciccarello reviewed Oct 17, 2016

View reviewed changes

Remove unused filesGlob and exclude from tsconfig

a573370

Refer to @internal TypeScript enum value using instead of copying it

ed77b41

mhegazy reviewed Oct 18, 2016

View reviewed changes

christopherthielen added 2 commits October 18, 2016 19:54

Re-implement hasProperty and normalizePath

37db93e

Replace IntrinsicType.intrinsicName with program.getTypeChecker().typ…

9491132

…eToString()

blakeembrey reviewed Oct 19, 2016

View reviewed changes

Move @types from devDependencies to dependencies

99ab42b

blakeembrey merged commit 603a5ff into TypeStrong:master Oct 20, 2016

christopherthielen mentioned this pull request Oct 20, 2016

Typedoc 0.5.0+ should use Typescript 2.0 "@types/*" #309

Closed

crazytoucan mentioned this pull request Nov 28, 2016

@types in dependencies causing build failures #350

Closed

aciccarello mentioned this pull request Jan 16, 2017

Moved @types to devDependencies. #384

Closed

blakeembrey mentioned this pull request Feb 8, 2017

Installing typedoc via npm installs @types/node #408

Closed

aciccarello mentioned this pull request Feb 24, 2017

Make @types packages dev dependencies. #434

Closed

aciccarello mentioned this pull request Apr 3, 2017

Fix separator in getDefaultLibFileName #465

Merged

This was referenced Jul 15, 2017

Use private TypeScript APIs from internal module #314

Merged

fix: remove specific version of typescript from dependencies #556

Closed

aciccarello mentioned this pull request Aug 17, 2018

Review ts-internal #830

Closed

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

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

Conversation

christopherthielen commented Oct 16, 2016

aciccarello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey commented Oct 18, 2016

aciccarello commented Oct 18, 2016

christopherthielen commented Oct 18, 2016

christopherthielen commented Oct 18, 2016 • edited Loading

blakeembrey commented Oct 18, 2016

MichaReiser commented Oct 18, 2016

christopherthielen commented Oct 18, 2016 • edited Loading

felixfbecker commented Oct 18, 2016 • edited Loading

christopherthielen commented Oct 18, 2016

felixfbecker commented Oct 18, 2016

blakeembrey commented Oct 18, 2016

christopherthielen commented Oct 18, 2016 • edited Loading

blakeembrey commented Oct 18, 2016 • edited Loading

christopherthielen commented Oct 18, 2016

christopherthielen commented Oct 18, 2016

felixfbecker commented Oct 18, 2016

christopherthielen commented Oct 18, 2016

blakeembrey commented Oct 18, 2016

mhegazy commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Oct 18, 2016

christopherthielen commented Oct 18, 2016

christopherthielen commented Oct 18, 2016

mhegazy commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherthielen Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey commented Oct 20, 2016

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

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

christopherthielen commented Oct 18, 2016 •

edited

Loading

christopherthielen commented Oct 18, 2016 •

edited

Loading

felixfbecker commented Oct 18, 2016 •

edited

Loading

christopherthielen commented Oct 18, 2016 •

edited

Loading

blakeembrey commented Oct 18, 2016 •

edited

Loading

christopherthielen Oct 19, 2016 •

edited

Loading