Skip to content

feat(@angular/cli): making app root configurable #6079

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

feat(@angular/cli): making app root configurable #6079

wants to merge 2 commits into from

Conversation

sumitarora
Copy link
Contributor

@sumitarora sumitarora commented Apr 25, 2017 •
edited
Loading

Fixes: #5927

hansl
hansl suggested changes Apr 25, 2017
View reviewed changes
packages/@angular/cli/blueprints/component/index.ts Outdated
@@ -112,7 +112,8 @@ export default Blueprint.extend({
resolveModulePath(options.module, this.project, this.project.root, appConfig);
} else {
try {
this.pathToModule = findParentModule(this.project.root, appConfig.root, this.generatePath);
this.pathToModule = findParentModule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parentheses on same line as function. Same everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hansl done

Brocco
Brocco suggested changes Apr 25, 2017
View reviewed changes
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:
If you're updating angular-cli.json you should also update the schema. The default will be needed for backwards compatability.
Tests?

@sumitarora sumitarora force-pushed the feat-approot-config branch from 0f79e8f to f34777f Compare April 25, 2017 19:17
@sumitarora
Copy link
Contributor Author

@Brocco Added Test 😄

@clydin
Copy link
Member

clydin commented Apr 25, 2017 •
edited
Loading

Looking at the blueprint, what is currently not configurable is the main ng module name (I.e, the bootstrapped module) and component name. A module's containing directory is also the name of the module so allowing configurability should allow the underlying issue's use case.
The "root" setting really is the app root as a large amount of app specific files are located there (index.html, main.ts, etc.).

@hansl hansl changed the base branch from master to 1.1.x April 25, 2017 23:50
@Brocco
Copy link
Contributor

Brocco commented Apr 26, 2017

Can you please rebase & repush this PR? There's a lot of noise in the diffs to review.

@sumitarora sumitarora force-pushed the feat-approot-config branch from abecf30 to c50a689 Compare April 26, 2017 13:20
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@sumitarora sumitarora force-pushed the feat-approot-config branch from c50a689 to 586b6be Compare April 26, 2017 18:16
@googlebot
Copy link

CLAs look good, thanks!

@sumitarora
Copy link
Contributor Author

@hansl @Brocco Updated and rebased 😄

@hansl hansl changed the base branch from 1.1.x to master May 4, 2017 00:31
@hansl
Copy link
Contributor

hansl commented May 4, 2017

Please rebase.

@sumitarora sumitarora force-pushed the feat-approot-config branch from 586b6be to af59849 Compare May 4, 2017 01:14
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels May 4, 2017
@sumitarora sumitarora force-pushed the feat-approot-config branch from af59849 to 9dfecb2 Compare May 4, 2017 13:36
@sumitarora sumitarora removed the cla: no label May 4, 2017
@sumitarora sumitarora force-pushed the feat-approot-config branch from 9dfecb2 to 08e7c97 Compare May 5, 2017 14:05
@googlebot googlebot added cla: no and removed cla: yes labels May 5, 2017
@sumitarora sumitarora force-pushed the feat-approot-config branch from 08e7c97 to a64c57d Compare May 24, 2017 15:16
@sumitarora sumitarora force-pushed the feat-approot-config branch from a64c57d to cc5c7bb Compare May 26, 2017 13:17
@hansl
Copy link
Contributor

hansl commented Sep 27, 2017

Closing every Blueprints related issue and PR.

@hansl hansl closed this Sep 27, 2017
This was referenced Oct 5, 2017
Closed
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hansl hansl hansl requested changes

@Brocco Brocco Brocco requested changes

@filipesilva filipesilva Awaiting requested review from filipesilva

Assignees

@Brocco Brocco

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ng generate <blueprint> assumes top level component is named "app"
5 participants
@sumitarora @clydin @Brocco @googlebot @hansl