-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build(aio): add i18n boilerplate type #20004
Conversation
@@ -10,7 +10,7 @@ const BOILERPLATE_BASE_PATH = path.resolve(SHARED_PATH, 'boilerplate'); | |||
const BOILERPLATE_COMMON_BASE_PATH = path.resolve(BOILERPLATE_BASE_PATH, 'common'); | |||
const EXAMPLES_BASE_PATH = path.resolve(__dirname, '../../content/examples'); | |||
|
|||
const BOILERPLATE_PATHS = { | |||
let BOILERPLATE_PATHS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to make this let
as you don't replace the actual object; you just modify a property of it.
@@ -45,6 +45,13 @@ const BOILERPLATE_PATHS = { | |||
] | |||
}; | |||
|
|||
const cliRelativePath = BOILERPLATE_PATHS.cli.map(file => `../cli/${file}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here explaining what this is doing will be helpful in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things to tweak but LGTM
38194fa
to
94c5d8d
Compare
Thanks for the review. I addessed the comments. But if you prefer a different comment, let me know, I struggled a bit :P |
94c5d8d
to
a20ecb5
Compare
Also removed the changes to the i18n example-config, that wasn't supposed to be there, yet. |
You can preview a20ecb5 at https://pr20004-a20ecb5.ngbuilds.io/. |
"tslint": "~5.3.2", | ||
"typescript": "~2.4.2" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it documented somewhere which files need to be update when we want to update dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it is updated. We could do that in a separate pr
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. |
This is an upgrade to aio boilerplate mechanism. For #19975 @ocombe needs a custom package.json for his guide. Instead of replicating the entire CLI boilerplate to just override one file, it is better to "inherit" from the CLI one and override the needed file. That way we don't have to update the boilerplate twice.
This needs to be reviewed and merged ASAP to release the docs early this week.