Improving null safety: Add FinalizedBsConfig and tweak plugin events#1000
Conversation
src/Program.ts
Outdated
| * The root directory for this program | ||
| */ | ||
| public options: BsConfig, | ||
| public options: FinalizedBsConfig, |
There was a problem hiding this comment.
If you'd like this to be a backwards-compatible change we could let BsConfig be passed here, and then apply normalizeConfig inside of the constructor. This feels a little weird to me, because it's letting potentially invalid data get pretty far into the program before getting normalized, but I can't think of any concrete cases where it would be harmful.
There was a problem hiding this comment.
Yeah. Let's just declare options as a field in the class body instead of here, and let the incoming type be BsConfig for the constructor. We're already doing a normalizeConfig in the constructor anyway.
There was a problem hiding this comment.
Sounds good to me. I've fixed this and reverted all of the test changes that were necessitated by the interface change.
This has significantly shrunk this PR. Should I keep it small or should I roll in the rest of the changes I have waiting in the wings? (I have a branch off of this branch that fixes a few hundred more issues.) I'm not sure how you want to group these changes since some of them may be breaking changes to the externally-published type definitions.
There was a problem hiding this comment.
Let's keep this PR as-is, and open new ones with other stuff. Feel free to open those other PRs targeting this one so you don't have to keep remembering which one is next. They'll get back in sync as we merge.
josephjunker
force-pushed
the
junker/null-safety-3
branch
from
e7eb8b0 to
6d6d95e
Compare
TwitchBronBron
left a comment
TwitchBronBron
left a comment
There was a problem hiding this comment.
Got a few suggestions, but overall looking good!
src/Program.ts
Outdated
| this.options.rootDir = util.getRootDir(this.options); | ||
|
|
||
| this.createGlobalScope(); | ||
| this.globalScope = this.createGlobalScope(); |
There was a problem hiding this comment.
What was the purpose of changing this to an assignment? The global scope was created in createGlobalScope.
There was a problem hiding this comment.
This was to make it easier to convince TS that this was indeed created in the constructor. As an alternative I could declare the scope property on the class as public globalScope: Scope = undefined as any, which might be less disruptive.
There was a problem hiding this comment.
Updated to public globalScope: Scope = undefined as any
src/ProgramBuilder.ts
Outdated
| */ | ||
| public get rootDir() { | ||
| if (!this.program) { | ||
| throw new Error('Cannot call `ProgramBuilder.rootDir` until after `ProgramBuilder.run()`'); |
There was a problem hiding this comment.
| throw new Error('Cannot call `ProgramBuilder.rootDir` until after `ProgramBuilder.run()`'); | |
| throw new Error('Cannot access `ProgramBuilder.rootDir` until after `ProgramBuilder.run()`'); |
TwitchBronBron
left a comment
TwitchBronBron
left a comment
There was a problem hiding this comment.
Got a few suggestions, but overall looking good!
TwitchBronBron
left a comment
TwitchBronBron
left a comment
There was a problem hiding this comment.
Everything looks great. Thanks for this!
2 participants
This only reduces the total number of null check errors by 30, but it pushes the errors a little farther towards the edges. I've resolved most of the errors in
ProgramandProgramBuilder. I believe this may be a breaking change, if plugins are allowed to callnew Program(), because the interface toProgram's constructor is now more strict.I'm a little unsure of what to do about
FinalizedBsConfig.rootDirandFinalizedBsConfig.stagingDir. I'd like to make them required, because a number of places assume that they exist. AFAICT their default values come from roku-deploy though, rather than anywhere inbsc, and if I add the default values from roku-deploy tonormalizeConfig, tons of tests break. I'm figuring that it's better to make incremental progress by making the other fields required than it is to risk breakage by making a more significant change to runtime behavior.I've added a unit test that asserts that the return value of
normalizeConfigdeeply matches an exact object. I added this test and confirmed that it was passing before making any changes tonormalizeConfig. This is to give some level of confidence that behavior is unchanged, even though I've had to update the configs in a lot of other tests to satisfy the more strict types.The changes to the plugin interface are as I mentioned in Slack, and shouldn't be more restrictive than what already exists. I made the name a little prettier than what I originally used in the Slack thread.