Reduce reliance on internal TS apis #949
Conversation
| } | ||
|
|
||
| return true; | ||
| return node.getChildren().some(ts.isModuleBlock); |
There was a problem hiding this comment.
Toss namespace A.B.C {} into https://ts-ast-viewer.com/ and it's immediately obvious that a module declaration is "topmost" if it has a module block as an immediate child.
| this.owner.convertNode(context, element); | ||
|
|
||
| if (_ts.isBindingPattern(element.name)) { | ||
| if (!ts.isBindingElement(element)) { |
There was a problem hiding this comment.
This additional check is required since the internal isBindingPattern check also ensures that node exists, while the is*BindingPattern do not
There was a problem hiding this comment.
Would it be better to inline this with the if block below?
| @@ -30,6 +30,7 @@ export class VariableConverter extends ConverterNodeComponent<ts.VariableDeclara | |||
|
|
|||
| /** | |||
| * Analyze the given variable declaration node and create a suitable reflection. | |||
| * TODO: the type of `node` is incorrect, it should be a union of ts.PropertySignature | ts.PropertyDeclaration | ... | |||
There was a problem hiding this comment.
Leaving resolving this to a later PR since changing to the actual type caused quite a few type errors.
| @@ -72,15 +72,6 @@ export class TypeScriptSource extends OptionsComponent { | |||
| } | |||
| } | |||
|
|
|||
| switch (option.paramType) { | |||
There was a problem hiding this comment.
No longer necessary since this is only used when printing help output, and TypeDoc hasn't printed TypeScript options in help since #569 was resolved months ago.
src/lib/converter/converter.ts
Outdated
| @@ -281,11 +274,12 @@ export class Converter extends ChildableComponent<Application, ConverterComponen | |||
| * @param fileNames Array of the file names that should be compiled. | |||
| */ | |||
| convert(fileNames: string[]): ConverterResult { | |||
| // TODO: This mutates the input array, is this side effect used anywhere? Can it be removed? | |||
There was a problem hiding this comment.
😬 That seems unexpected and if it is intentional it shouldn't be done here. I wonder if we can use a map here.
There was a problem hiding this comment.
It doesn't look like the side effects are used anywhere since this is passed into Application
| @@ -65,6 +58,29 @@ function getRootModuleDeclaration(node: ts.ModuleDeclaration): ts.Node { | |||
| return node; | |||
| } | |||
|
|
|||
| /** | |||
| * Derived from the internal ts utility | |||
| * https://github.com/Microsoft/TypeScript/blob/v3.2.2/src/compiler/utilities.ts#L954 | |||
There was a problem hiding this comment.
Thanks for including the source link
| @@ -5,6 +5,13 @@ import { Component } from '../../component'; | |||
| import { DiscoverEvent, OptionsComponent } from '../options'; | |||
| import { ParameterType } from '../declaration'; | |||
|
|
|||
| enum CharacterCodes { | |||
There was a problem hiding this comment.
Should we add a source link?
| this.owner.convertNode(context, element); | ||
|
|
||
| if (_ts.isBindingPattern(element.name)) { | ||
| if (!ts.isBindingElement(element)) { |
There was a problem hiding this comment.
Would it be better to inline this with the if block below?
Summary of changes:
ts.CompilerHostimplementation - it is not required when creating a program and we just passed the calls tots.sysfunctions (and a few internal apis) anyways.I'll also make a few inline comments where code was just removed since it was no longer necessary.