-
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(serve): Persist serve options in angular-cli.json #3908
feat(serve): Persist serve options in angular-cli.json #3908
Conversation
|
e1211e5
to
c80108c
Compare
}; | ||
serve?: { |
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 block was the only thing truly added. The rest was whitespace reindentation (editorconfig).
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 because generating the schema.d.ts
is not done manually. You need to run npm run build-config-interface
.
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.
Now I see the $comment
! Thanks 😄
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 few nits. In general looks good to me.
}; | ||
serve?: { |
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 because generating the schema.d.ts
is not done manually. You need to run npm run build-config-interface
.
const defaultPort = process.env.PORT || 4200; | ||
const defaultLiveReloadPort = config.get('defaults.serve.liveReloadPort') || 49152; | ||
const defaultPort = process.env.PORT || config.get('defaults.serve.port'); | ||
const defaultHost = config.get('defaults.serve.host') || 'localhost'; |
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.
config.get
should be avoided in the code. It removes typing information as it returns any
.
@@ -54,6 +54,11 @@ | |||
"module": false, | |||
"pipe": true, | |||
"service": true | |||
}, | |||
"serve": { |
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.
Since you already have good default values, those are not needed and just add noise to this file. I'd like the default config to grow slimmer.
7062118
to
8baa45e
Compare
f92bf97
to
339c8b5
Compare
@hansl can you re-review? |
81206d7
to
0905cff
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.
LGTM, just one more thing.
"type": "object", | ||
"properties": { | ||
"port": { | ||
"type": "number", |
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 need to add description
for all new properties. We're going to do a full run through existing ones, but new properties should have proper descriptions. This is because we're going to add support for IDEs that support schema, and those descriptions will be shown to users. Also, the config documentation will be generated from these.
0905cff
to
b32835a
Compare
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. |
Fixes #1156