refactor(all): enable strictPropertyInitialization in TsConfig and fix the affected code
#29
Conversation
|
@nicanac once this PR is merged, I think you can release the next beta version 😉 |
ok, nice, thanks |
| public ngOnInit(): void { | ||
| console.log(componentName + ": controller initialized"); | ||
|
|
||
| public constructor(protected translateService: TranslateService) { | ||
| this.selectedLanguage = this.translateService.currentLang; | ||
|
|
||
| this.languageChangeSubscription = this.translateService.onLangChange.subscribe( |
There was a problem hiding this comment.
Small question, in the file "src/app/app.component.ts", you keep the subscriptions in the ngOnInit().
Here, you move them in the constructor. Why ?
There was a problem hiding this comment.
Ah, I thought it might be better to keep those subscriptions in the ngOnInit in the AppComponent because they are related to the BreakpointObserver... which is about the media queries stuff and maybe doing that in the constructor is too soon cause the component is not rendered yet...
But that is just a guess... I have no strong opinion about it... maybe I can simply move them to the constructor too
There was a problem hiding this comment.
@SuperITMan I've updated the AppComponent to move the subscriptions to the constructor and it seems to work fine. So it is now consistent with the other components 😉
There was a problem hiding this comment.
Okido 😊
I didn't think to the BreakpointObserver but if it works correctly in the constructor(), it's fine.
I think this library is ready to be released in "1.0.0" 😃
What do you think @nicanac @christophercr ? 😊
There was a problem hiding this comment.
Mmm not sure if we should release "1.0.0"... maybe an RC? It is just because there are 2 tasks that we can still work on for the final release: issues 28 and 21
What do you guys think? Or... we could also release those in "1.1.0"...
There was a problem hiding this comment.
Yes, I think we can do an RC.
There was a problem hiding this comment.
Okido ! Let's go for the RC 😊
…fix the affected code
christophercr
force-pushed
the
feature/ts-strict-property-initialization
branch
from
a14a7ee to
843d739
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The
strictPropertyInitializationrule is disabled in ourtsconfig, so this is not aligned to the compilation rules used in the other NBB packages i.e. Stark.What is the new behavior?
The
strictPropertyInitializationrule is now enabled and all the affected code has been adapted to comply with it.Does this PR introduce a breaking change?