Strict null checks! #845

Gerrit0 · 2018-08-27T03:35:46Z

This touches a... lot of code. Fortunately, most of it is trivial changes.

The highlights:

Turned on strictNullChecks, strictPropertyInitialization in tsconfig.json
Turned on no-for-in-array, strict-type-predicates in tslint.json

Needs attention:

I changed the tsconfig target from es5 to es2015. This change was made in order to make the ReflectionFlags class act as a class. Since the lib property already includes es2015 libraries, and we only test on node 6+, I believe this is a justifiable change.
In order to make the .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?)
I've added several interfaces marked as internal in the Typescript library to replace a single interface which was inaccurate. I'd like to remove the reliance on internal apis, but it works for now...

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

aciccarello

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!

aciccarello · 2018-09-26T01:18:06Z

src/lib/application.ts

@@ -67,21 +67,21 @@ export class Application extends ChildableComponent<Application, AbstractCompone
        defaultValue: 'console',
        type: ParameterType.Mixed
    })
-    loggerType: string|Function;
+    readonly loggerType!: string|Function;


Why are these marked readonly?

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.

I think we should remove readonly to avoid unnecessary changes for anyone who may be touching the javascript apis.

aciccarello · 2018-09-26T01:20:45Z

src/lib/application.ts

@@ -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);


Why wouldn't we want the owner to be nullable?

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.

Would setting the owner to itself be an alternative to using a symbol and needing to complicate the type?

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.

Okay, I'd like to double back on this, possibly at a later time to find a cleaner solution. For now it works.

aciccarello · 2018-09-26T02:26:27Z

src/lib/converter/context.ts


    /**
     * The currently set type arguments.
     */
-    typeArguments: Type[];
+    typeArguments?: (Type | undefined)[];


Why is undefined allowed in the array?

It looks like I missed this when cleaning up, there's no reason that it should be this way. Fixing.

aciccarello · 2018-09-26T03:11:22Z

src/lib/converter/context.ts

            const name = declaration.symbol.name;
            if (this.typeArguments && this.typeArguments[index]) {
-                typeParameters[name] = this.typeArguments[index];
+                typeParameters[name] = this.typeArguments[index]!;


that's annoying that this doesn't type automatically 😞

It does once typeArguments type is corrected to just Type[]

aciccarello · 2018-09-26T03:18:24Z

src/lib/converter/convert-expression.ts

@@ -12,7 +12,7 @@ export function convertDefaultValue(node: ts.VariableDeclaration|ts.ParameterDec
    if (node.initializer) {
        return convertExpression(node.initializer);
    } else {
-        return null;
+        return '';


I think this should be undefined and the function signature be string | undefined

I agree, fixing.

src/lib/converter/nodes/accessor.ts

aciccarello · 2018-09-26T05:34:07Z

src/lib/converter/types/array.ts

@@ -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.


Oops! Fixed.

aciccarello · 2018-09-26T05:37:12Z

src/lib/converter/converter.ts

+            }
+        });
+        return result;
+    }


This was a good idea. 👍

Closes TypeStrong#864

closes TypeStrong#846

NaridaL · 2018-10-06T20:48:56Z

src/lib/utils/events.ts


    /**
     * A unique id that identifies this instance.
     */
-    private _listenId: string;
+    private _listenId!: string;


I think this can be _listenId?, as the uses on lines 408 and 415 check for null?

NaridaL · 2018-10-06T20:51:43Z

src/lib/utils/events.ts

@@ -249,12 +254,12 @@ export class Event {
    /**
     * Has [[Event.stopPropagation]] been called?
     */
-    private _isPropagationStopped: boolean;
+    private _isPropagationStopped?: boolean;


Why not just : boolean = false;?

aciccarello

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.

src/lib/utils/options/declaration.ts

aciccarello · 2018-10-12T05:59:48Z

src/lib/utils/options/declaration.ts

 }

+/**
+ * TODO: This should be a union type of multiple possible option types.
+ */


👍 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.

aciccarello

Only change I'd like to see is removing the readonly's. Looking forward to merging this to master!

aciccarello · 2018-10-18T14:52:27Z

src/lib/application.ts

@@ -67,21 +67,21 @@ export class Application extends ChildableComponent<Application, AbstractCompone
        defaultValue: 'console',
        type: ParameterType.Mixed
    })
-    loggerType: string|Function;
+    readonly loggerType!: string|Function;


I think we should remove readonly to avoid unnecessary changes for anyone who may be touching the javascript apis.

aciccarello · 2018-10-18T14:53:47Z

src/lib/application.ts

@@ -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);


Okay, I'd like to double back on this, possibly at a later time to find a cleaner solution. For now it works.

Gerrit0 · 2018-10-18T17:33:34Z

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...

aciccarello · 2018-10-18T18:00:35Z

Just on node 6? That's odd.

Gerrit0 · 2018-10-18T22:04:47Z

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.

Gerrit0 · 2018-10-19T05:24:48Z

Apparently highlight.js recently had an update which changed how language auto-detection works. The tests included some code blocks which did not specify a language, and so since the auto-detected language was different, highlight.js gave different output, causing a test failure.

Apparently npm 6.4.1 did not upgrade to the latest minor version (following package-lock.json) while npm 4.6.1 did. To resolve this, I pinned highlight.js to 9.12.0 which should have resolved the problem. I believe there may be a caching issue on the CI server as I was able to see all tests pass with node v6.14.4 and npm v4.6.1 locally.

aciccarello · 2018-10-19T23:55:06Z

Attempting to restart the CI

aciccarello · 2018-10-20T02:39:02Z

@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

…015 in TypeStrong/typedoc#845

christopherthielen · 2019-01-10T00:02:22Z

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 _super.call(this), but you have to new an ES6 class.

aciccarello · 2019-01-10T00:30:19Z

👍 @christopherthielen Thanks for sharing that info. I'll update the release notes to communicate this.

webmaster128 · 2019-01-11T10:12:13Z

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.

aciccarello · 2019-01-11T13:32:43Z

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?

Apparently highlight.js recently had an update which changed how language auto-detection works. The tests included some code blocks which did not specify a language, and so since the auto-detected language was different, highlight.js gave different output, causing a test failure.

Apparently npm 6.4.1 did not upgrade to the latest minor version (following package-lock.json) while npm 4.6.1 did. To resolve this, I pinned highlight.js to 9.12.0 which should have resolved the problem. I believe there may be a caching issue on the CI server as I was able to see all tests pass with node v6.14.4 and npm v4.6.1 locally.

See #845 (comment)

aciccarello · 2019-01-12T00:24:03Z

@webmaster128 Highlight.js has been updated on master and will be included in the next release.

…015 in TypeStrong/typedoc#845

Refs: TypeStrong/typedoc#845 (comment)

Gerrit0 and others added 8 commits August 25, 2018 15:59

Turn strictNullChecks on

cc401fa

Remove yarn error log, add to .gitignore

752d8cf

Restore name splitting behavior

8f90d12

Fix missing implemented interfaces

1735d4d

TypeStrong#837 Add contributing.md (TypeStrong#841)

820077a

* Sets newLine config

Use a dummy readme when testing the renderer (TypeStrong#857)

589f15a

aciccarello reviewed Sep 26, 2018

View reviewed changes

Gerrit0 and others added 5 commits September 26, 2018 09:32

Peer review comments

eeccb6b

fix(HelperStack): deactivate theme resources correctly (TypeStrong#865)

f7d5683

Closes TypeStrong#864

Upgrade typescript to v3.1.x (TypeStrong#863)

f0166cf

fix: No error is thrown when adding plug-ins repeatedly(TypeStrong#847)

1108878

closes TypeStrong#846

0.13.0-0

b371fd4

NaridaL reviewed Oct 6, 2018

View reviewed changes

aciccarello added 3 commits October 10, 2018 07:53

0.13.0

7247605

simplify event test types

ee162c9

simplify types

daab024

aciccarello reviewed Oct 12, 2018

View reviewed changes

aciccarello and others added 10 commits October 12, 2018 08:19

correct getter in EventDispatcher

5759cb5

Turn strictNullChecks on

20c90c7

Remove yarn error log, add to .gitignore

179f5af

Restore name splitting behavior

e1bb10c

Fix missing implemented interfaces

fbe3a5a

Peer review comments

fc192ce

simplify event test types

e110441

simplify types

4dcf9b1

correct getter in EventDispatcher

4bf87f6

aciccarello mentioned this pull request Oct 18, 2018

Review boolean declaration option parsing #885

Closed

aciccarello requested changes Oct 18, 2018

View reviewed changes

Remove readonly on option properties

5517705

Pin highlight.js version to 9.12.0

4d4ff46

aciccarello closed this Oct 19, 2018

aciccarello reopened this Oct 19, 2018

aciccarello merged commit a2fab7c into TypeStrong:master Oct 20, 2018

Gerrit0 deleted the strict-null-checks branch October 20, 2018 04:38

mcaden added a commit to mcaden/typedoc that referenced this pull request Oct 24, 2018

Revert "Strict null checks! (TypeStrong#845)"

909600a

This reverts commit a2fab7c. Reverting to attempt to correct bug where flags not being written

christopherthielen added a commit to christopherthielen/typedoc-plugin-external-module-name that referenced this pull request Jan 9, 2019

fix(*): Switch to emitting ES2015 to account for Typedoc emitting ES2…

00a4ae8

…015 in TypeStrong/typedoc#845

webmaster128 mentioned this pull request Jan 11, 2019

Upgrade Typescript to 3.2 iov-one/iov-core#684

Merged

Gerrit0 added a commit that referenced this pull request Jan 11, 2019

Update & unpin highlight.js

8994f4b

See #845 (comment)

Gerrit0 mentioned this pull request Jan 11, 2019

Update & unpin highlight.js #946

Merged

aciccarello pushed a commit that referenced this pull request Jan 12, 2019

Update & unpin highlight.js (#946)

fc23c25

See #845 (comment)

christopherthielen added a commit to christopherthielen/typedoc-plugin-internal-external that referenced this pull request Jan 17, 2019

fix(*): Switch to emitting ES2015 to account for Typedoc emitting ES2…

37d707c

…015 in TypeStrong/typedoc#845

christopherthielen added a commit to christopherthielen/typedoc-plugin-ui-router that referenced this pull request Jan 17, 2019

fix(*): Switch to emitting ES2015 to account for Typedoc emitting ES2…

1e249ed

…015 in TypeStrong/typedoc#845

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade typedoc-plugin-external-module-name from 2.0.0 to 2.2.1 Kappy-Technologies-LLP/botkit#1

Open

Gerrit0 mentioned this pull request Mar 18, 2020

Useless getter in Application class #1235

Closed

1 task

trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Jul 30, 2020

fix: add target=ES2015 for client-documentation-generator

1bff288

Refs: TypeStrong/typedoc#845 (comment)

Strict null checks! #845

Strict null checks! #845

Conversation

Gerrit0 commented Aug 27, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NaridaL Oct 6, 2018 • edited

Choose a reason for hiding this comment

aciccarello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Gerrit0 commented Oct 18, 2018

aciccarello commented Oct 18, 2018

Gerrit0 commented Oct 18, 2018

Gerrit0 commented Oct 19, 2018

aciccarello commented Oct 19, 2018

aciccarello commented Oct 20, 2018

christopherthielen commented Jan 10, 2019 • edited

aciccarello commented Jan 10, 2019

webmaster128 commented Jan 11, 2019

aciccarello commented Jan 11, 2019

aciccarello commented Jan 12, 2019

NaridaL Oct 6, 2018 •

edited

christopherthielen commented Jan 10, 2019 •

edited