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
Enable TypeScript strictNullChecks #2755
Conversation
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.
src/Chunk.ts
Outdated
@@ -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; |
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.
Logic change. Shouldn't change any behavior.
@@ -33,6 +33,7 @@ export default class BlockStatement extends StatementBase { | |||
for (const node of this.body) { | |||
if (node.hasEffects(options)) return true; | |||
} | |||
return 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.
Logic change. It seems unlikely that this would affect anything.
src/ast/nodes/BreakStatement.ts
Outdated
@@ -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)) |
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.
Logic change. Alternative would be to add an as boolean
.
@@ -48,10 +48,9 @@ export default class ExportDefaultDeclaration extends NodeBase { | |||
|
|||
initialise() { | |||
this.included = false; | |||
const declaration = this.declaration as FunctionDeclaration | ClassDeclaration; |
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.
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
.
@@ -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)); |
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.
Logic change. Alternative would be to add an as boolean
.
src/utils/chunkColouring.ts
Outdated
@@ -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 }; |
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.
Logic change, same as above.
@@ -95,8 +96,8 @@ export default class Import extends NodeBase { | |||
} | |||
} | |||
|
|||
setResolution(interop: boolean, namespace: string = undefined): void { | |||
setResolution(interop: boolean, namespace?: string): void { |
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.
Type change. This function is only used in this file and it should be safe.
import Variable from '../variables/Variable'; | ||
import ChildScope from './ChildScope'; | ||
|
||
export default class Scope { | ||
children: ChildScope[] = []; | ||
variables: { | ||
[name: string]: Variable; | ||
arguments?: ArgumentsVariable; |
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.
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?
@@ -30,7 +30,7 @@ interface RawMemberDescription { | |||
|
|||
function assembleMemberDescriptions( | |||
memberDescriptions: { [key: string]: RawMemberDescription }, | |||
inheritedDescriptions: MemberDescriptions = null | |||
inheritedDescriptions: MemberDescriptions | null = null |
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.
Type change. This function is only used within this file so this should be safe.
@@ -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 |
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.
Type change. This function is only used within this file and is already handling the undefined
case anyway.
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 |
# 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
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 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.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: Fixes #2051
Description
This sets
strictNullChecks
totrue
intsconfig.json
, which causes TypeScript to be stricter around possiblynull
orundefined
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:
foo!.bar
) because I felt that full type assertions were more noticeable and easier to grep for.as
type assertions over<typename>(value)
type assertions. It looks like both are in use in the code base, but my impression is thatas
is generally preferred. (It's my personal preference.)boolean
and I went with!!
overBoolean
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.