Conversation
| templateUrl: '<%= dasherizedModuleName %>.component.html', | ||
| styleUrls: ['<%= dasherizedModuleName %>.component.<%= styleExt %>'] | ||
| }) | ||
| export class <%= classifiedModuleName %>Component { |
There was a problem hiding this comment.
I'd suggest to implement the OnInit interface if we're defining ngOnInit method.
There was a problem hiding this comment.
@mgechev I agree with that (didn't know it existed) but do you agree with having ngOnInit defined by default in a component? I think we should have something other than a constructor in there so devs don't see that as the primary place to put code
There was a problem hiding this comment.
Makes sense to me. Also I noticed that the VSCode snippets by @johnpapa declare ngOnInit as well.
On the other hand, I haven't used this hook that often in the components that I've developed and if I was using the CLI in most cases I'd have to remove the ngOnInit declaration.
There was a problem hiding this comment.
It's a good point. We dont want to add things people will remove.
without ngOnInit I have noticed many people end up putting that startup code in the ctor. That makes it awful to test. That's why I like it there form a template standpoint. But if you have no startup code, you would need to remove it. I almost always use it.
That said, I'd be OK removing ngOnInit and the interface and the import if everyone here wants it that way.
There was a problem hiding this comment.
I think it comes down to the use case, what fraction of generated components will end up needing/using this? Unfortunately at this point we don't have enough of a sample size to make a decision based upon facts/research only based upon gut feeling and the risk of devs being aware of the lifecycle hooks for components. I've seen enough examples of initialization code inside of the ctor, which is what lead me to add this in the first place.
Personally I think it should be there (with the interface reference also added), but I am also OK with it not being there at all.
There was a problem hiding this comment.
I also prefer it being there. And the interface always goes with it, IMO. Otherwise folks are apt to change a spelling of it and then it never runs. Uh oh
I vote with Mike and say keep it for now.
There was a problem hiding this comment.
Yes, I prefer it to be there as well. Seems like a great suggestion for making your code more testable. In the end, if you don't like it nothing stops you from deleting it from the generated component.
There was a problem hiding this comment.
Exactly and potentially down the road an option could be added to the generator to control what gets generated
Brocco
force-pushed
the
style-guide
branch
2 times, most recently
from
9746373 to
aeae925
Compare
Brocco
force-pushed
the
style-guide
branch
4 times, most recently
from
d8e3ed4 to
ddcee2f
Compare
|
|
||
| it('should ...', injectAsync([TestComponentBuilder], (tcb:TestComponentBuilder) => { | ||
| return tcb.createAsync(<%= classifiedModuleName %>).then((fixture) => { | ||
| return tcb.createAsync(<%= classifiedModuleName %>Component).then((fixture: ComponentFixture) => { |
There was a problem hiding this comment.
Is <%= classifiedModuleName %>Component better than adding Component to the moduleName instead? Just curious.
We could even do logic to see if the user is saying ng g component my-comp.component we don't add a second component to it. WDYT?
Brocco
force-pushed
the
style-guide
branch
2 times, most recently
from
75f1590 to
e942b98
Compare
|
LGTM assuming the Windows test are fixed. Good work! |
Implements the majority of style guide angular#316 Moves systemjs configuration our of index.html angular#429
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
4 participants
No description provided.