Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Fix lint/tsc issues when the strict tsc flag is true #464

Conversation

mattfurness
Copy link
Contributor

Recently the Stacktrace team have found issues in their spas/libs that could have been caught earlier by the tsc if strict was turned on, particularly strictNullChecks. If we try to turn on strict in our spas/libs we get errors that originate in skyux-builder because the builder is built in to the spas/libs. This PR aims to do the least amount of work possible to allow consumers to turn on strict if they choose.

It fixes the linting issues without changing any functionality or public / private api of skyux builder, mostly by:

@codecov-io
Copy link

codecov-io commented Sep 5, 2018

Codecov Report

Merging #464 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #464   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          53     53           
  Lines        1642   1642           
  Branches      241    241           
=====================================
  Hits         1642   1642
Flag Coverage Δ
#builder 100% <ø> (ø) ⬆️
#runtime 100% <ø> (ø) ⬆️
#srcapp 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
runtime/bootstrapper.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b868cf...f65c91b. Read the comment docs.

@mattfurness mattfurness force-pushed the support-consumers-with-strict-on branch 2 times, most recently from bcd61aa to a2a8b47 Compare September 5, 2018 04:06
@mattfurness mattfurness changed the title Fix lint/tsc issues when strict tsc flag is true Fix lint/tsc issues when the strict tsc flag is true Sep 5, 2018
@Blackbaud-PaulCrowder
Copy link
Member

@Blackbaud-BobbyEarl I took a look and I think I'm fine with these changes, but I'd like to get your take on it as well.

@mattfurness mattfurness force-pushed the support-consumers-with-strict-on branch from a2a8b47 to b074824 Compare September 7, 2018 04:30
@Blackbaud-BobbyEarl
Copy link

Hey @mattfurness , can you clarify how I might best see these errors prior to your changes? I ran skyux new, updated the tsconfig.json file to include strict and strictNullChecks in the compilerOptions section. I do get a couple errors, but none that would seem addressed by your PR.

@mattfurness
Copy link
Contributor Author

The errors I found and fixed here were from turning on strict temporarily in this repo and running the tests. I have created a temporary PR in the internal BB sky-lib-p2p-events repo that shows around 11 errors from builder in the failedskyux test step. My hunch is that you need to import from @blackbaud/skyux-builder/runtime explicitly to start seeing the errors.

@bentefay bentefay force-pushed the support-consumers-with-strict-on branch 2 times, most recently from d0a4cf3 to f33a3ec Compare October 9, 2018 06:52
This commit aims to fix any issues when the `strict` flag is on without changing
any of the current behavior, or public api at all. The idea is that no consumer
will be adversely affected by this change. It will allow consumers to opt in
to `strict` without any issues from skyux builder.
@bentefay bentefay force-pushed the support-consumers-with-strict-on branch from f33a3ec to f65c91b Compare October 15, 2018 01:19
@bentefay
Copy link
Contributor

bentefay commented Oct 15, 2018

Hey @Blackbaud-BobbyEarl. I work with @mattfurness on the Stacktrace team at Blackbaud. We noticed that you have updated a bunch of the files in @blackbaud/skyux-builder/runtime to import from other precompiled npm packages. Fantastic work! This has resolved most of the compile errors that result from setting strict: true.

There is one remaining error, which we have updated our PR to fix. You can reproduce this error yourself by running skyux new, setting compilerOptions": { "strict": true } in tsconfig.json, and then updating HomeComponent to import SkyuxConfig as follows:

import { Component } from '@angular/core';
import { SkyAppConfig } from '@blackbaud/skyux-builder/runtime';

@Component({
  selector: 'my-home',
  templateUrl: './home.component.html'
})
export class HomeComponent {
  constructor(config: SkyAppConfig) {
    console.log(config);
  }
}

When running skyux lint, you should see the error:

Error at /Users/ben/Code/matt/skyux-spa-test/node_modules/@blackbaud/skyux-builder/runtime/bootstrapper.ts:31:13: Argument of type 'string[] | { [key: string]: boolean | { value?: any; required?: boolean | undefined; excludeFromR...' is not assignable to parameter of type 'SkyuxConfigParams'.
  Type 'undefined' is not assignable to type 'SkyuxConfigParams'.

Our PR fixes this issue. We'd really appreciate you accepting this PR, as strict: true will significantly improve our ability to handle null and undefined properly.

Thanks a lot!

@Blackbaud-BobbyEarl
Copy link

@mattfurness and @bentefay. First off, my sincerest apologies for being unresponsive on this issue.

Thanks for hanging with us. This LGTM, but will look for additional validation from @Blackbaud-PaulCrowder and @Blackbaud-SteveBrush.

@Blackbaud-PaulCrowder Blackbaud-PaulCrowder merged commit fe6d4be into blackbaud:master Oct 16, 2018
@bentefay bentefay deleted the support-consumers-with-strict-on branch October 17, 2018 05:39
Blackbaud-MikitaYankouski pushed a commit to Blackbaud-MikitaYankouski/skyux-builder that referenced this pull request May 3, 2019
This commit aims to fix any issues when the `strict` flag is on without changing
any of the current behavior, or public api at all. The idea is that no consumer
will be adversely affected by this change. It will allow consumers to opt in
to `strict` without any issues from skyux builder.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants