-
Notifications
You must be signed in to change notification settings - Fork 12k
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
feat(command): adding the --app command option #4754
Conversation
b246573
to
6cb983b
Compare
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.
First comments.
@@ -0,0 +1,11 @@ | |||
export function getAppFromConfig(apps: any, name: String) { |
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.
We have typing for the configuration.
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.
There is interface for whole config not apps part.
@@ -2,9 +2,10 @@ var path = require('path'); | |||
var process = require('process'); | |||
var fs = require('fs'); | |||
|
|||
module.exports = function dynamicPathParser(project, entityName) { | |||
module.exports = function dynamicPathParser(project, entityName, appConfig) { | |||
|
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.
Remove empty line.
@@ -1,3 +1,5 @@ | |||
import * as appUtils from '../../utilities/app-utils'; |
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.
Instead of import * as
, just import the function you need. This is a TypeScript file, not a CommonJS one.
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.
Here and everywhere.
@@ -2,11 +2,13 @@ import * as fs from 'fs'; | |||
import * as path from 'path'; | |||
const SilentError = require('silent-error'); | |||
|
|||
export default function findParentModule(project: any, currentDir: string): string { | |||
const sourceRoot = path.join(project.root, project.ngConfig.apps[0].root, 'app'); | |||
export default function findParentModule |
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.
Put as many arguments on the same line as function declaration, then indent 4 (or left align with first argument) the rest.
this.pathToModule = path.join(this.project.root, parsedPath.dir, parsedPath.base); | ||
|
||
if (!fs.existsSync(this.pathToModule)) { | ||
throw 'Module specified does not exist'; | ||
} | ||
} else { | ||
try { | ||
this.pathToModule = findParentModule(this.project, this.dynamicPath.dir); | ||
this.pathToModule = findParentModule |
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.
Put as many arguments on the same line as function declaration, then indent 4 (or left align with first argument) the rest.
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.
Here and everywhere.
7e884e9
to
4fed19c
Compare
2422388
to
428957f
Compare
@@ -54,7 +56,9 @@ const BuildCommand = Command.extend({ | |||
aliases: ['b'], | |||
|
|||
availableOptions: baseBuildCommandOptions.concat([ | |||
{ name: 'stats-json', type: Boolean, default: false } | |||
{ name: 'watch', type: Boolean, default: false, aliases: ['w'] }, |
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.
Watch is now part of the base options, I think you left it here as part of a bad rebase.
@@ -24,6 +24,7 @@ const E2eCommand = Command.extend({ | |||
baseServeCommandOptions.concat([ | |||
{ name: 'config', type: String, aliases: ['c'] }, | |||
{ name: 'specs', type: Array, default: [], aliases: ['sp'] }, | |||
{ name: 'app', type: String, aliases: ['a'] }, |
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.
Add it to baseBuildCommandOptions instead, and build/serve/e2e will all use it.
@@ -0,0 +1,11 @@ | |||
export function getAppFromConfig(apps: any, name: String) { |
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.
This looks like a pretty good candidate for a static method on models/config
. What do you think?
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.
it's used by karma.js
too so it's better as independent util function
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.
I changed the whole testing config setup in the last day so it should be much more like the others.
@@ -11,11 +13,18 @@ export default Blueprint.extend({ | |||
name: 'spec', | |||
type: Boolean, | |||
description: 'Specifies if a spec file is generated.' | |||
}, | |||
{ | |||
name: 'app', |
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.
This gets repeated a lot. It's probably better to make some kind of base BaseAppOptions
+AppOptions
similar to baseBuildCommandOptions
+BuildOptions
(or a better abstraction if you can find it).
That way if we need to change it, or add more app options in the future, it's only in one place.
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.
sure we should do it as a refactoring task as it needs to be done in multiple commands
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.
Let's do it separately.
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.
If we're doing that we should also be doing the same thing with things like flat
and spec
too
dacbfd5
to
0487edb
Compare
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 |
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.
1 more nit.
@@ -0,0 +1,13 @@ | |||
import {CliConfig as CliConfigInterface} from '../lib/config/schema'; | |||
|
|||
export function getAppFromConfig(apps: CliConfigInterface['apps'], name: String) { |
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.
as a fallback, if the name can be used as a number (using name.match(/^[0-9]+$/)
), use parseInt(name, 10)
to get it. This way users can do ng build --app=1
and get the first app.
d1c30e1
to
55ab668
Compare
CLAs look good, thanks! |
ce07732
to
b213d72
Compare
} | ||
]; | ||
|
||
export interface BuildTaskOptions extends BuildOptions { | ||
watch?: boolean; |
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.
This line is not needed, watch is in BuildOptions
;
} | ||
]; | ||
|
||
export interface BuildTaskOptions extends BuildOptions { | ||
watch?: boolean; | ||
app?: String; |
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.
@@ -0,0 +1,11 @@ | |||
export function getAppFromConfig(apps: any, name: String) { |
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.
I changed the whole testing config setup in the last day so it should be much more like the others.
@@ -16,7 +17,12 @@ function isDirectory(path: string) { | |||
} | |||
|
|||
const init: any = (config: any) => { | |||
const appConfig = CliConfig.fromProject().config.apps[0]; | |||
// load Angular CLI config |
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.
This isn't needed anymore, I think you left it here as part of the rebase.
@@ -23,6 +23,7 @@ export interface ServeTaskOptions extends BuildOptions { | |||
sslCert?: string; | |||
open?: boolean; | |||
hmr?: boolean; | |||
app?: string; |
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.
Remove app from here and instead add it to https://github.com/angular/angular-cli/blob/master/packages/@angular/cli/models/build-options.ts.
7a31214
to
839b9ce
Compare
I try to do ng build --app 1 where 1 is the index of a configuration inside the "apps" field, but it seesms not work, what do i do wrong? "apps": [ |
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. |
Added option for
ng serve --app <app_name>
to commandsbuild
,serve
,generate
,eject
,i18n