Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

feat(@schematics/angular): save ng new options as defaults #666

Merged
merged 1 commit into from
Apr 13, 2018
Merged

feat(@schematics/angular): save ng new options as defaults #666

merged 1 commit into from
Apr 13, 2018

Conversation

cyrilletuzi
Copy link
Contributor

Reintroduced #196 in schematics for CLI v6.

Fixes #615

@filipesilva @hansl @Brocco Could you review and merge this, so it it's part of the v6 final release? Thanks.

@chrste90
Copy link
Contributor

Do you think this is the right location to save the options? I know that it is working to set these options on schematics but shouldn't these be set at application / library scope?

For example the --prefix options isn't working too and there is an open PR (#644) which will set the tslint rules to allow different prefixes for libs and apps. I think these defaults here could also be different for apps and libs.

@filipesilva filipesilva requested a review from Brocco April 10, 2018 17:14
@filipesilva
Copy link
Contributor

@Brocco can you have a look?

@cyrilletuzi
Copy link
Contributor Author

cyrilletuzi commented Apr 10, 2018

Don't think the bazel failure is due to this PR.

About the location of the defaults options, the ng g library doesn't contain the same option as ng new. So if the defaults are only in the app project options, users would have to redo the config again for each lib (and the new syntax is complex). And I think we can assume someone doing a library alongside an app project will use the same defaults. Otherwise, the lib would be a distinct project.

// tslint:disable-next-line:no-any
const schematics: any = {};

if (options.inlineTemplate !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

none of these should ever be undefined since the schema contains default values for all the options referenced here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? I supposed options was the ones passed in the cli command, as the types are x | undefined. If so, it's just a quick fix, I will correct the PR later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@chrste90
Copy link
Contributor

@cyrilletuzi Yes, sounds reasonable :)

I created a separate PR for prefixes: #685

@@ -223,6 +223,36 @@ function addAppToWorkspaceFile(options: ApplicationOptions, workspace: Workspace
// }

workspace.projects[options.name] = project;

// tslint:disable-next-line:no-any
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove any, use JsonObject instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used any because it is what is done just above for project. Changed to JsonObject as request in last commit, but forced me to cast each property.

const schematics: any = {};

if (options.inlineTemplate === true
|| options.inlineStyle === true
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent by 4 spaces.

@cyrilletuzi
Copy link
Contributor Author

@hansl requested changes done :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants