fix(cli): remove copyright header from generated app #991
Conversation
use `.template` extension for templates. Rename during project generation. Removes copyright headers from templates. Fixes #944
virkt25
requested review from
bajtos,
kjdelisle,
raymondfeng and
shimks
as code owners
| @@ -55,6 +56,24 @@ module.exports = class ProjectGenerator extends BaseGenerator { | |||
| const isValid = utils.validate(this.args[0]); | |||
| if (typeof isValid === 'string') throw new Error(isValid); | |||
| } | |||
|
|
|||
| this.setupRenameTransformer(); | |||
There was a problem hiding this comment.
Is this line of code needed? I thought functions defined in Yeoman index files get called in sequential order they're defined in.
There was a problem hiding this comment.
Yep. If I comment it out and run CLI, the transformer is not registered ...
| */ | ||
| setupRenameTransformer() { | ||
| this.registerTransformStream( | ||
| rename(function(file) { |
There was a problem hiding this comment.
Is there a function that can do this for us without having to import another package? I guess it's not a huge deal regardless though
There was a problem hiding this comment.
We can write a function ourselves ... but I didn't want to re-invent the wheel. (And it requires dealing with streams ... )
| setupRenameTransformer() { | ||
| this.registerTransformStream( | ||
| rename(function(file) { | ||
| if (file.extname === '.template') { |
There was a problem hiding this comment.
It's probably better to narrow it to *.js.template and *.ts.template.
There was a problem hiding this comment.
file.extname property only gives us access to the last .extname in the basename of a file. I can do a check again file.basename instead but I think that makes the code messy.
There was a problem hiding this comment.
We can do the following:
const fileName = `${file.basename}.${file.extname}`;
const result = fileName.match(/(.+)\.(ts|js)\.template$/);
if (result) {
file.extname = result[2];
file.basename = result[1];
}
There was a problem hiding this comment.
I agree that we shouldn't lean on it being *.template, since it means someone could put a non-TS/JS template in there and we would mistakenly rewrite the results as a .ts or .js file.
There was a problem hiding this comment.
(I don't know how much we really should worry or care about that, but the cost of not letting others be silly in that way is minimal.)
| setupRenameTransformer() { | ||
| this.registerTransformStream( | ||
| rename(function(file) { | ||
| if (file.extname === '.template') { |
There was a problem hiding this comment.
I agree that we shouldn't lean on it being *.template, since it means someone could put a non-TS/JS template in there and we would mistakenly rewrite the results as a .ts or .js file.
|
@raymondfeng Feedback applied. PTAL. (Had to change the Regular Expression slightly to get the leading |
|
Nice! Have you @virkt25 considered using |
|
@bajtos I did, it was proposed by you in the original task as well and would’ve been my preferred approach but the final task requirements in #944 asked the templates be switched to a |
Yes please! (On a side on, I think we should allow more time for reviews. For example, I am listed as a CODEOWNER for |
use
.templateextension for templates. Rename during project generation. Removes copyright headers from templates.Fixes #944
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated