-
Notifications
You must be signed in to change notification settings - Fork 712
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
Strict null checks! #845
Strict null checks! #845
Conversation
Also updates launch.json and tasks.json to be more helpful in VSCode and enables downlevelIteration. In order to fix some of the unit tests, the target was updated to es2015 - since the lib already references es2015 libraries and we are only testing on node 6+, I believe this is acceptable.
* Sets newLine config
Adding localization plugin in the plugin section. This plugin will ease every user, to: 1. Generate a json files with all code comments based on the project structure 2. Edit the json with the translation on the desired language 3. Re-generate the typodoc documentation with the updated jason for the new language
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being slow to respond. I only was able to get partially through the PR but would like to start the feedback loop early. I see some improvements could be made to avoid !
nullable assertions but as I've thought about it, those improvements might be suited for another PR.
Overall this should help make the code easier to reason about so thanks for putting this together!
src/lib/application.ts
Outdated
@@ -67,21 +67,21 @@ export class Application extends ChildableComponent<Application, AbstractCompone | |||
defaultValue: 'console', | |||
type: ParameterType.Mixed | |||
}) | |||
loggerType: string|Function; | |||
readonly loggerType!: string|Function; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these marked readonly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe my thought process was that they should only be set by option providers, not manually, it would be fine to remove the readonly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove readonly to avoid unnecessary changes for anyone who may be touching the javascript apis.
@@ -94,7 +94,7 @@ export class Application extends ChildableComponent<Application, AbstractCompone | |||
* @param options An object containing the options that should be used. | |||
*/ | |||
constructor(options?: Object) { | |||
super(null); | |||
super(DUMMY_APPLICATION_OWNER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't we want the owner to be nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for .owner
finds 161 uses, none of which handle nullable owners. In interest of avoiding more changes, I decided to leave it non-nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would setting the owner to itself be an alternative to using a symbol and needing to complicate the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Typescript allowed super(this)
, it would be. However since this isn't allowed, we either need to do super(undefined as any)
and add the owner manually, or use a symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'd like to double back on this, possibly at a later time to find a cleaner solution. For now it works.
src/lib/converter/context.ts
Outdated
|
||
/** | ||
* The currently set type arguments. | ||
*/ | ||
typeArguments: Type[]; | ||
typeArguments?: (Type | undefined)[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is undefined allowed in the array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I missed this when cleaning up, there's no reason that it should be this way. Fixing.
src/lib/converter/context.ts
Outdated
const name = declaration.symbol.name; | ||
if (this.typeArguments && this.typeArguments[index]) { | ||
typeParameters[name] = this.typeArguments[index]; | ||
typeParameters[name] = this.typeArguments[index]!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's annoying that this doesn't type automatically 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does once typeArguments
type is corrected to just Type[]
@@ -12,7 +12,7 @@ export function convertDefaultValue(node: ts.VariableDeclaration|ts.ParameterDec | |||
if (node.initializer) { | |||
return convertExpression(node.initializer); | |||
} else { | |||
return null; | |||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be undefined and the function signature be string | undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, fixing.
src/lib/converter/types/array.ts
Outdated
@@ -29,7 +29,7 @@ export class ArrayConverter extends ConverterTypeComponent implements TypeConver | |||
/** | |||
* Convert the given array type node to its type reflection. | |||
* | |||
* This is a node based converter with no type equivalent. | |||
* This is a node based convert er with no type equivalent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Fixed.
} | ||
}); | ||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a good idea. 👍
src/lib/utils/events.ts
Outdated
|
||
/** | ||
* A unique id that identifies this instance. | ||
*/ | ||
private _listenId: string; | ||
private _listenId!: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be _listenId?
, as the uses on lines 408 and 415 check for null?
src/lib/utils/events.ts
Outdated
@@ -249,12 +254,12 @@ export class Event { | |||
/** | |||
* Has [[Event.stopPropagation]] been called? | |||
*/ | |||
private _isPropagationStopped: boolean; | |||
private _isPropagationStopped?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just : boolean = false;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was able to complete more review. Just a couple comments at this point. I'd like to get this large PR merged so it doesn't conflict with any other work. At some point in the futuer, I'd like to get rid of the Symbol designating the application but that can be done in a separate PR. Thanks for the hard work.
} | ||
|
||
/** | ||
* TODO: This should be a union type of multiple possible option types. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This can be done later.
Also updates launch.json and tasks.json to be more helpful in VSCode and enables downlevelIteration. In order to fix some of the unit tests, the target was updated to es2015 - since the lib already references es2015 libraries and we are only testing on node 6+, I believe this is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only change I'd like to see is removing the readonly's. Looking forward to merging this to master!
src/lib/application.ts
Outdated
@@ -67,21 +67,21 @@ export class Application extends ChildableComponent<Application, AbstractCompone | |||
defaultValue: 'console', | |||
type: ParameterType.Mixed | |||
}) | |||
loggerType: string|Function; | |||
readonly loggerType!: string|Function; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove readonly to avoid unnecessary changes for anyone who may be touching the javascript apis.
@@ -94,7 +94,7 @@ export class Application extends ChildableComponent<Application, AbstractCompone | |||
* @param options An object containing the options that should be used. | |||
*/ | |||
constructor(options?: Object) { | |||
super(null); | |||
super(DUMMY_APPLICATION_OWNER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'd like to double back on this, possibly at a later time to find a cleaner solution. For now it works.
Not entirely sure why that build failed, there was no functional change made in the last commit. At least it also fails locally so I have something to go on... |
Just on node 6? That's odd. |
Yea, this is very odd. I had v9.10.0 on my machine, after upgrading to the 10.12.0, I get a different file causing the failure... Absolutely no clue where something broke. |
Apparently Apparently npm |
Attempting to restart the CI |
@Gerrit0 Thanks for digging into that. That's frustrating that there was a change on a minor version... |
This reverts commit a2fab7c. Reverting to attempt to correct bug where flags not being written
Note that I had to switch to emitting es2015 in a typedoc plugin to support Typedoc 0.14.0. This is because typedoc classes are now emitted as ES6 classes and my plugin extends one of those classes. The plugin was emitting ES5 which uses |
👍 @christopherthielen Thanks for sharing that info. I'll update the release notes to communicate this. |
Why does this pin highlight.js to 0.12.0? The upgrade typedoc 0.13.x -> 0.14.1 now downgrades my highlight.js from 9.13.1 to 9.12.0. |
It seems like hightligh.js made some changes to language auto-detection that changed tests and npm wasn't resolving consistently. @Gerrit0 could highlight.js be bumped and unpinned and the tests updated in a separate PR?
|
@webmaster128 Highlight.js has been updated on master and will be included in the next release. |
This touches a... lot of code. Fortunately, most of it is trivial changes.
The highlights:
Needs attention:
.application
and.owner
properties in components type correctly, I added a symbol which is used to identify the root component, an alternative choice would be to make the.owner
and.application
properties nullable, which might be desirable if components will ever be used without an application (possible?)