[Core] Enforce strictNullChecks #453
Conversation
mayurkale22
requested review from
draffensperger and
justindsmith
as code owners
mayurkale22
force-pushed
the
enforce_strictNullcheck_core
branch
from
d655fdb
to
b4b1112
Compare
There was a problem hiding this comment.
This is great and represents a significant improvement in type safety for the library!!
I made a number of comments, but I do think that getting this enabled is important so if you don't have the time to do all the fixes, I could be open to committing this and then doing follow-up refactors over time.
| @@ -39,21 +38,21 @@ export abstract class BasePlugin implements types.Plugin { | |||
| /** The module name */ | |||
| protected moduleName: string; | |||
| /** A tracer object. */ | |||
| protected tracer: modelTypes.Tracer; | |||
| protected tracer!: modelTypes.Tracer; | |||
There was a problem hiding this comment.
Normally I would discourage ! for properties like this, but I think it is worth it to get the strict null checks enforced.
I think ideally these would be initialized in the constructor for the plugin, but I'm guessing that would involve a bunch more refactoring.
There was a problem hiding this comment.
Currently users need to pass tracer instance, when enabling the plugins. It would need more work and breaking change in order to initialize same in the constructor.
packages/opencensus-core/src/trace/model/no-record/no-record-span-base.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-core/src/trace/model/no-record/no-record-span-base.ts
Outdated
Show resolved
Hide resolved
packages/opencensus-core/src/trace/model/no-record/no-record-span-base.ts
Outdated
Show resolved
Hide resolved
| @@ -246,7 +246,7 @@ export interface SpanOptions { | |||
| parentSpanId?: string; | |||
| } | |||
|
|
|||
| export type TraceState = string; | |||
| export type TraceState = string|undefined; | |||
There was a problem hiding this comment.
What would you think about instead making the places that use TraceState explicitly include |undefined (maybe even with a find/replace)?
I think making a type include undefined or null is kind of risks confusing people reading the code (and maybe even bugs) because when they see a type that naturally assume it wouldn't be null/undefined.
1. Assign default value to parentSpanId and name.
2. Assign default value({}) to activeTraceParams.
3. Add traceState back for no-record-span-base
Codecov Report
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 94.89% 94.66% -0.23%
==========================================
Files 148 150 +2
Lines 9850 9737 -113
Branches 701 723 +22
==========================================
- Hits 9347 9218 -129
- Misses 503 519 +16
Continue to review full report at Codecov.
|
|
@draffensperger Finally, a build is passed and all comments are resolved. PTAL |
| @@ -39,21 +38,21 @@ export abstract class BasePlugin implements types.Plugin { | |||
| /** The module name */ | |||
| protected moduleName: string; | |||
| /** A tracer object. */ | |||
| protected tracer: modelTypes.Tracer; | |||
| protected tracer!: modelTypes.Tracer; | |||
Fixes #348. This was the last package remaining for the enforcing
strictNullChecksoption.