Enable TypeScript strictNullChecks #2755

edsrzf · 2019-03-16T08:41:27Z

This PR contains:

Are tests included?

yes
no

Breaking Changes?

yes
no

List any relevant issue numbers: Fixes #2051

Description

This sets strictNullChecks to true in tsconfig.json, which causes TypeScript to be stricter around possibly null or undefined values.

There were 400 errors to fix and most of them were fixed by adding explicit type assertions at every error site, as discussed in #2051. It would be a good idea to go back and clean some of these up afterward.

I tried to avoid changing any actual logic or types, but there are a few places that I either had to or simply couldn't resist. I'll try to point them all out in the pull request comments.

Some stylistic things:

I avoided using the non-null assertion operator (foo!.bar) because I felt that full type assertions were more noticeable and easier to grep for.
I went for as type assertions over <typename>(value) type assertions. It looks like both are in use in the code base, but my impression is that as is generally preferred. (It's my personal preference.)
There are a few places I added conversions to boolean and I went with !! over Boolean since I saw more instances of the former in the code base already.

I tried enabling full strict mode, but that resulted in an extra 315 errors, so I think I'll have a look at that separately.

There were 400 errors to fix and most of the mwere fixed by adding explicit type assertions at every error site. It would be a good idea to go back and clean some of these up later. There are a few places that I changed the logic or the types, which I've tried to point out in the pull request comments.

edsrzf · 2019-03-16T08:42:23Z

src/Chunk.ts

@@ -706,7 +711,7 @@ export default class Chunk {
 	): boolean {
 		const seen = new Set<Chunk | ExternalModule>();
 		function visitDep(dep: Chunk | ExternalModule): boolean {
-			if (seen.has(dep)) return;
+			if (seen.has(dep)) return false;


Logic change. Shouldn't change any behavior.

edsrzf · 2019-03-16T08:43:02Z

src/ast/nodes/BlockStatement.ts

@@ -33,6 +33,7 @@ export default class BlockStatement extends StatementBase {
 		for (const node of this.body) {
 			if (node.hasEffects(options)) return true;
 		}
+		return false;


Logic change. It seems unlikely that this would affect anything.

edsrzf · 2019-03-16T08:43:36Z

src/ast/nodes/BreakStatement.ts

@@ -11,7 +11,7 @@ export default class BreakStatement extends StatementBase {
 		return (
 			super.hasEffects(options) ||
 			!options.ignoreBreakStatements() ||
-			(this.label && !options.ignoreLabel(this.label.name))
+			!!(this.label && !options.ignoreLabel(this.label.name))


Logic change. Alternative would be to add an as boolean.

edsrzf · 2019-03-16T08:46:46Z

src/ast/nodes/ExportDefaultDeclaration.ts

@@ -48,10 +48,9 @@ export default class ExportDefaultDeclaration extends NodeBase {

 	initialise() {
 		this.included = false;
+		const declaration = this.declaration as FunctionDeclaration | ClassDeclaration;


Added an intermediate variable because the type assertions seemed to confuse the compiler so it didn't to realize that the id property could never be null.

edsrzf · 2019-03-16T08:47:08Z

src/ast/nodes/ExportNamedDeclaration.ts

@@ -23,7 +23,7 @@ export default class ExportNamedDeclaration extends NodeBase {
 	}

 	hasEffects(options: ExecutionPathOptions) {
-		return this.declaration && this.declaration.hasEffects(options);
+		return !!(this.declaration && this.declaration.hasEffects(options));


Logic change. Alternative would be to add an as boolean.

edsrzf · 2019-03-16T08:50:53Z

src/utils/chunkColouring.ts

@@ -78,7 +78,7 @@ Try defining "${chunkName}" first in the manualChunks definitions of the Rollup
 		}
 		handledEntryPoints[currentEntry.id] = true;
 		currentEntryHash = randomUint8Array(10);
-		modulesVisitedForCurrentEntry = { [currentEntry.id]: null };
+		modulesVisitedForCurrentEntry = { [currentEntry.id]: false };


Logic change, same as above.

edsrzf · 2019-03-16T08:51:20Z

src/ast/nodes/Import.ts

@@ -95,8 +96,8 @@ export default class Import extends NodeBase {
 		}
 	}

-	setResolution(interop: boolean, namespace: string = undefined): void {
+	setResolution(interop: boolean, namespace?: string): void {


Type change. This function is only used in this file and it should be safe.

edsrzf · 2019-03-16T08:52:34Z

src/ast/scopes/Scope.ts

 import Variable from '../variables/Variable';
 import ChildScope from './ChildScope';

 export default class Scope {
 	children: ChildScope[] = [];
 	variables: {
 		[name: string]: Variable;
-		arguments?: ArgumentsVariable;


Type change. The compiler complained that these properties had a different type than the index signature. The alternative would've been to make the index signature [name: string]: Variable | undefined;, which introduced errors elsewhere.

This change didn't introduce any new type errors so should be okay?

edsrzf · 2019-03-16T08:52:58Z

src/ast/values.ts

@@ -30,7 +30,7 @@ interface RawMemberDescription {

 function assembleMemberDescriptions(
 	memberDescriptions: { [key: string]: RawMemberDescription },
-	inheritedDescriptions: MemberDescriptions = null
+	inheritedDescriptions: MemberDescriptions | null = null


Type change. This function is only used within this file so this should be safe.

edsrzf · 2019-03-16T08:53:19Z

src/utils/addons.ts

@@ -9,7 +9,9 @@ export interface Addons {
 	outro?: string;
 }

-function evalIfFn(strOrFn: string | (() => string | Promise<string>)): string | Promise<string> {
+function evalIfFn(
+	strOrFn: string | (() => string | Promise<string>) | undefined


Type change. This function is only used within this file and is already handling the undefined case anyway.

lukastaegert · 2019-03-18T11:38:20Z

Thanks a lot! It seems this change breaks quite a lot of tests, though. Maybe some of the explicit type conversions had unintended side-effects? Note that especially with plugins, there are quite a few situations where null or undefined are not the same as false, but I did not investigate further yet.

# Conflicts: # src/Chunk.ts # src/ExternalModule.ts # src/Graph.ts # src/Module.ts # src/finalisers/amd.ts # src/finalisers/cjs.ts # src/finalisers/iife.ts # src/finalisers/umd.ts # src/rollup/index.ts # src/utils/assetHooks.ts # src/utils/chunkColouring.ts # src/utils/defaultPlugin.ts # src/utils/pluginDriver.ts # tsconfig.json

lukastaegert

I reverted most logic changes until everything worked, not sure which one it was. I also updated to latest master, will merge once CI is happy.

edsrzf commented Mar 16, 2019

View reviewed changes

lukastaegert added 5 commits May 17, 2019 16:44

Revert logic changes to fix tests

07f0b10

Fix remaining missing types

4e4642b

Enforce consistent type-casting style

fa74345

Explicitly list strict options we are still missing

4906448

lukastaegert approved these changes May 17, 2019

View reviewed changes

lukastaegert merged commit 65b6aef into rollup:master May 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TypeScript strictNullChecks #2755

Enable TypeScript strictNullChecks #2755

edsrzf commented Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

edsrzf Mar 16, 2019

lukastaegert commented Mar 18, 2019

lukastaegert left a comment

Enable TypeScript strictNullChecks #2755

Enable TypeScript strictNullChecks #2755

Conversation

edsrzf commented Mar 16, 2019

Description

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

lukastaegert commented Mar 18, 2019

lukastaegert left a comment

Choose a reason for hiding this comment