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 (again) #487

Conversation

bentefay
Copy link
Contributor

@bentefay bentefay commented Oct 18, 2018

Hey guys, thanks for getting #464 merged and released! Unfortunately, the additional commit that was included in the 1.25 release (d4c34b7) happened to introduce a new TSLint error, which means we still can't turn on Typescript's strict mode. This PR fixes that error.

Obviously this approach of us post-hoc fixing strict mode compile errors is not really sustainable long term. However, in the short term, we'd really like to get at least one release of skyux-builder where we can switch strict mode on. Hence this PR. If you could possibly create a patch release with just this commit, it would be fantastic! We can then stay on that version until we can come up with a more sustainable solution. For the longer term, I'm happy to create an issue to discuss some possible permanent solutions, but just as an example we could:

  1. Turn on strict mode for the local build of skyux-builder. We would have to be careful not to change the default tsconfig.json, as this is referenced in the skyux new template, and we do not want to effect all consuming projects.
  2. An even better solution would be to compile the typescript files in skyux-builder/runtime. That way typescript files in skyux-builder couldn't effect consumers of the library, allowing them to use whatever typescript compiler settings (and ideally, version) that they like.

Any thoughts?

Additional Info

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 .../@blackbaud/skyux-builder/runtime/i18n/host-locale-provider.ts:39:14: Variable 'locale' is used before being assigned.

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.
@codecov-io
Copy link

codecov-io commented Oct 18, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #487   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          54     54           
  Lines        1666   1666           
  Branches      247    247           
=====================================
  Hits         1666   1666
Flag Coverage Δ
#builder 100% <ø> (ø) ⬆️
#runtime 100% <ø> (ø) ⬆️
#srcapp 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
runtime/i18n/host-locale-provider.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 96a6a8c...2c39779. Read the comment docs.

@jamesottaway
Copy link

Thanks Bobby for re-queuing the failed build step. Now that this is green is it possible to merge and release a patch version with this change included today?

@Blackbaud-PaulCrowder Blackbaud-PaulCrowder merged commit 9332e83 into blackbaud:master Oct 24, 2018
@Blackbaud-PaulCrowder
Copy link
Member

@jamesottaway I'm working on a release now.

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
4 participants