Update to TypeScript 2.2.1 #448
Conversation
tsconfig.json
Outdated
| "lib": [ | ||
| "DOM", | ||
| "ES5", | ||
| "ScriptHost", |
There was a problem hiding this comment.
What is ScriptHost used for?
There was a problem hiding this comment.
It's a standard include w/ an ES5 target. It's possible we don't need it, but I am not familiar enough to remove a prior dependency.
https://github.com/Microsoft/TypeScript/blob/master/lib/lib.scripthost.d.ts
There was a problem hiding this comment.
No it's not. That looks like IE-specific extensions and definitely isn't ES spec.
There was a problem hiding this comment.
Yes. It is mostly IE-specific extensions. It's not part of the ES spec. It is included when --target: ES5 (and ES6) is used. See https://www.typescriptlang.org/docs/handbook/compiler-options.html under the --lib row.
Note: If --lib is not specified a default library is injected. The default library injected is:
► For --target ES5: DOM,ES5,ScriptHost
Anyway, I removed ScriptHost locally and it compiled. Ambient declarations from definitely typed don't typically reference it, so we should be OK removing it.
There was a problem hiding this comment.
What references is irrelevant here - we shouldn't be including any libraries we don't have. We shouldn't even include dom since we're not in the DOM. All that does is give a false sense of security and errors at runtime instead.
There was a problem hiding this comment.
I think that if lib is not specified in tsconfig.json, then dom is part of the default libs brought in per https://www.typescriptlang.org/docs/handbook/compiler-options.html and DOM is necessary in this project because highlight.js is a dependency.
There was a problem hiding this comment.
Removing DOM fails the build:
Running "ts:typedoc" (ts) task
Compiling...
Using tsc v2.2.1
node_modules/@types/highlight.js/index.d.ts(19,40): error TS2304: Cannot find name 'Node'.
There was a problem hiding this comment.
Yeah, that's a known issue with highlight.js - I believe we've mentioned it somewhere - maybe wherever we moved to @types. Fixing that is sadly contingent on fixing the definition in DefinitelyTyped or enable TypeScript to understand browser resolution/do conditional definition paths (since there's no Node constructor in node).
src/lib/utils/options/declaration.ts
Outdated
| @@ -46,7 +46,7 @@ export class OptionDeclaration { | |||
|
|
|||
| scope: ParameterScope; | |||
|
|
|||
| map: Object; | |||
| protected map: Object | Map<string, string> | 'object'; | |||
There was a problem hiding this comment.
Can we avoid the protected change?
There was a problem hiding this comment.
Limiting the scope made it easier to understand where map is being used. Is there a reason to keep this value public?
There was a problem hiding this comment.
Yes - if someone is using it, you just broke them. Backward compatibility is important, even though it's minor. In any case, they'll probably break anyway since the type has changed, but this is really just an unnecessary change as a result.
There was a problem hiding this comment.
map is really an internal implementation detail of what OptionDeclaration does and shouldn't be public. If map remains public then anyone who relies on map will silently fail; because Map is also an Object TypeScript will not throw an error about incompatible types and map will look like an empty object. Changing map to protected ensures anyone who wants to continue to use map will need to extend this class and be aware of the change. Overall, we are better off making this breaking change and preventing silent failures for users.
src/lib/utils/options/declaration.ts
Outdated
| const key = value ? (value + '').toLowerCase() : ''; | ||
| try { | ||
| this.getMapValue(key); | ||
| } catch (error) { |
There was a problem hiding this comment.
Can we please avoid using try..catch statements here. It seems like we should be able to keep most of the code the same as it was instead of changing all this.
There was a problem hiding this comment.
I don't actually see where you'd updating value = anymore.
There was a problem hiding this comment.
Sure. I can make this change.
src/lib/utils/options/declaration.ts
Outdated
| errorCallback('Invalid value for option "%s".', this.name); | ||
| } | ||
| const message = this.mapError ? [this.mapError] : ['Invalid value for option "%s".', this.name]; | ||
| errorCallback(... message); |
There was a problem hiding this comment.
Can we keep this code the same as it was before, I don't think there's any need to be clever here.
Mapadded support todeclarationWill resolve #432, #414, and #417; maybe #344