-
Notifications
You must be signed in to change notification settings - Fork 12k
feat(@angular/cli): Update generate & new to use schematics #7090
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
Conversation
} | ||
], | ||
|
||
anonymousOptions: [ | ||
'<blueprint>' | ||
'<schemtatic>' |
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.
schemtatic
-> schematic
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.
Please fix.
@@ -34,117 +60,109 @@ export default Command.extend({ | |||
description: 'Run through without making any changes.' | |||
}, | |||
{ | |||
name: 'lint-fix', |
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.
--lint-fix
still needs to exist as an option.
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 is in and working
}, | ||
|
||
beforeRun: function(rawArgs: string[]) { | ||
const collection = getCollection(this.getCollectionName(rawArgs)); | ||
|
||
const isHelp = ['--help', '-h'].includes(rawArgs[0]); |
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.
Are you printing schematic description on --help
?
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.
Possibly, I want to get it all working first before worrying about tweaking the help output.
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 definitely want this.
@@ -58,7 +58,7 @@ export default function () { | |||
`)) | |||
.then(() => silentExec(normalize('node'), 'index.js')) | |||
.then(() => expectFileToMatch('dist/index.html', | |||
new RegExp('<h2 _ngcontent-c0="">Here are some links to help you start: </h2>'))) | |||
new RegExp('<h2>Here are some links to help you start: </h2>'))) |
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 looked at this with @Brocco and as far as I could tell, _ngcontent-c0=""
wasn't in the output of prerendering for the versions I tested. Prerendering was still happening correctly though.
I've been following this feature and addon support for a while now. This looks fantastic! Can't wait to try this out. 👍 👍 |
925c57e
to
f46caf2
Compare
62836b7
to
df6011e
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.
early comments.
@@ -32,7 +32,7 @@ export class CliConfig extends CliConfigBase<ConfigInterface> { | |||
const projectConfig = CliConfig.fromProject(); | |||
if (projectConfig) { | |||
value = projectConfig.get(jsonPath); | |||
} else if (CliConfig.globalConfigFilePath() !== CliConfig.configFilePath()) { | |||
} else { |
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.
Curious about this change. Why is this different now?
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 should be in a new PR... it breaks the way we pull config values.
} | ||
], | ||
|
||
anonymousOptions: [ | ||
'<blueprint>' | ||
'<schemtatic>' |
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.
Please fix.
import * as fs from 'fs'; | ||
import * as os from 'os'; | ||
import * as path from 'path'; | ||
import { cyan, yellow } from 'chalk'; |
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 seems like this should be a task, rather than changing the build command directly, and that the new
commands should reuse the task and pass in the different name for the schematics from the config.
1d38827
to
276f73d
Compare
let error = false; | ||
const loggingQueue: string[] = []; | ||
|
||
dryRunSink.reporter.subscribe((event: DryRunEvent) => { |
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'm going to have to add comments about schematics themselves here, unfortunately. New e2e are generated with @Brocco Any reason the |
And help remains a big factor. We need to support help right away. |
@hansl I don't see any
|
83eba07
to
f1e5b13
Compare
@@ -92,6 +92,7 @@ export default Task.extend({ | |||
}); | |||
|
|||
return new Promise((resolve, reject) => { | |||
console.log(opts); |
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.
debugging info?
32f4ec1
to
d314b8a
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.
Minor non-blocking comments, other than that LGTM.
}; | ||
preppedOptions = { | ||
...preppedOptions, | ||
...normalizeOptions(schematic.description.name, keys, options) |
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.
Shouldn't this be part of the previous declaration?
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.
no, because the options may have been updated from one statement to the next.
if (!commandOptions.dryRun) { | ||
process.chdir(commandOptions.directory); | ||
} | ||
}) | ||
.then(function () { | ||
if (!commandOptions.skipInstall) { |
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 looks like a lapse that install still happens on a dry run. If only schematicRunTask.run
is meant to happen on dry runs, it sounds better to wholly skip the promise chain after that when dry run is on.
} | ||
}) | ||
.then(() => ng('generate', 'component', 'test-component')) | ||
.then((output) => { | ||
if (!output.stdout.match(/identical src[\\|\/]app[\\|\/]app.module.ts/)) { | ||
if (!output.stdout.match(/ERROR! src[\\|\/]app[\\|\/]test-component[\\|\/]test-component.component.ts already exists./)) { |
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 could be a merge strategy problem in Schematics itself.
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.
The merge strategy is being passed in via the CLI... the only change to force (pun intended) this behavior is to change --force
's default to be true.
This feature is related to angular#6593
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.
LGTM
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 feature is related to #6593