feat(typescript): add option to disable leading comment concatenation #270
Conversation
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Thanks for submitting this @devversion.
I feel a little awkward about hard coding the license tag requirement in here. It would also be a breaking change (even if for the better for most projects).
How about we simplify the approach and just add an option for not concatenating leading comments? In practice, I don't think that we should be collecting up multiple documentation comments before the current AST node anyway. (At least I don't remember the motivation for doing this).
devversion
force-pushed
the
fix/typescript-license-header-comment
branch
2 times, most recently
from
fa27863 to
74cf6dd
Compare
|
|
||
| // Search for classes with a constructor marked as `@internal` | ||
| if (exportDoc.docType === 'class' && exportDoc.constructorDoc && exportDoc.constructorDoc.internal) { | ||
| const classDoc = exportDoc as ClassExportDoc; |
There was a problem hiding this comment.
We should rather use a type constraint function here:
function isPrivateClassExportDoc(doc as any): doc is ClassExportDoc {
return exportDoc.docType === 'class' && exportDoc.constructorDoc && exportDoc.constructorDoc.internal;
}
This would be called as:
if (isPrivateClassExportDoc(exportDoc)) {
...
}
Then inside the if statement the exportDoc will already be typed as ClassExportDoc
| export class Host { | ||
|
|
||
| /** Whether multiple leading comments for a TypeScript node should be concatenated. */ | ||
| concatMultipleLeadingComments: boolean = true; |
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
A minor suggestion around using a type constraint, but otherwise looks good!
|
@petebacondarwin Done. Thanks for your review. Please have another look. |
|
I just realize that the commit description and title are outdated. Do you want me to update them? Otherwise you can just change the title when merging? (should be easy when using |
devversion
force-pushed
the
fix/typescript-license-header-comment
branch
from
6f4415b to
613a12e
Compare
devversion
changed the title
|
@petebacondarwin I've squashed all commits and tried to create an appropriate description & title. Please have another look. Thanks! |
| // Register the services and file readers | ||
| .factory(modules) | ||
| .factory(exportSymbolsToDocsMap) | ||
| .factory('tsHost', () => new Host()) |
There was a problem hiding this comment.
You can get away with a fat arrow function here, because it has no dependencies. But this would not work if you added a dependency as a parameter. Therefore I think it is safer to avoid using these when doing DI.
There was a problem hiding this comment.
Actually you only get away with it because you are compiling the TS down to ES5, which downgrades the fat arrow to a normal JS function: () => new Host() becomes function() { return new Host(); }).
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Probably safer to avoid fat arrow functions in DI injectable factories but fine otherwise.
Introduces a new option that allows developers to configure the `tsHost` to not concatenate multiple leading comments for a `TypeScript` node content. The `tsHost` is a newly created service that delegates (for now) to the `tsParser` package for retrieving the content of a given node. The advantage of the `tsHost` is that people can inject the `tsHost` in Dgeni `.config()` and specify options or overwrite methods. Related angular/components#12736.
devversion
force-pushed
the
fix/typescript-license-header-comment
branch
from
613a12e to
64ad97e
Compare
|
Released as 0.26.6 |
|
@petebacondarwin Awesome. Thanks! |
3 participants
Introduces a new option that allows developers to configure the
tsHostto not concatenate multiple leading comments for aTypeScriptnode content.The
tsHostis a newly created service that delegates (for now) to thetsParserpackage for retrieving the content of a given node. The advantage of thetsHostis that people can inject thetsHostin Dgeni.config()and specify options or overwrite methods.Related angular/components#12736.