Skip to content

feat(@angular/cli): read proxyConfig from angular-cli.json - #6493

Merged
hansl merged 1 commit into
angular:masterfrom
ayvazj:issue6240
Jul 5, 2017
Merged

hansl merged 1 commit into
angular:masterfrom
ayvazj:issue6240

Conversation

@ayvazj

@ayvazj ayvazj commented May 28, 2017

Copy link
Copy Markdown
Contributor

easy proxy config by reading default from angular-cli.json (#6240)

@filipesilva filipesilva left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM overall, just a few minor changes to ensure everything is consistent. Good work!

"proxyConfig": {
"description": "Proxy configuration file.",
"type": "string",
"default": ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you remove the default here? It's better to not have one since sending in an empty string is different than not sending anything.

@@ -0,0 +1,59 @@
import * as express from 'express';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you rename the proxy.ts test to proxy-config.ts, and do all tests in a single file using the --proxy-config flag? I think we must have renamed the flag at one point and forgot to change the test name.

@filipesilva filipesilva self-assigned this Jun 1, 2017
@ayvazj
ayvazj force-pushed the issue6240 branch 2 times, most recently from 85e6daf to fe43113 Compare June 3, 2017 19:19
@ayvazj

ayvazj commented Jun 3, 2017

Copy link
Copy Markdown
Contributor Author

updated - let me know if I missed anything. Thanks.

@filipesilva

Copy link
Copy Markdown
Contributor

The tests seem to be failing, there might be something broken in the new set of changes.

@ayvazj

ayvazj commented Jun 13, 2017 •

Copy link
Copy Markdown
Contributor Author

tests are failing for me because the command @angular/cli is keeping a port open. Could it be that killAllProcesses is not working? The tree-kill node module is using pgrep -P PID on macOS which does not work, but ps ax does.

> sudo lsof -t -i:4200           
32159
> pgrep -P 32159
> ps -a 32159
PID TTY           TIME CMD
32159 ttys000    3:33.14 @angular/cli

Is this a known issue?

@filipesilva

Copy link
Copy Markdown
Contributor

@ayvazj I'm not sure what's keeping the port open on the revised test, but am reasonably sure killAllProcesses is working since we have a lot of tests that use it.

It might be that you can't do .then(() => expectToFail(() => ngServe())); at the end and need to do .then(() => expectToFail(() => ng('serve')) instead, since ngServe() is waiting to match some output.

easy proxy config by reading default from angular-cli.json (angular#6240)
@ayvazj

ayvazj commented Jul 4, 2017 •

Copy link
Copy Markdown
Contributor Author

@filipesilva got some time to revisit this. my bad, I wasn't opening/closing the server properly after consolidating tests, causing downstream tests to fail.

@filipesilva

Copy link
Copy Markdown
Contributor

@ayvazj thanks for updating it, there's a test failing but I think it's a fluke so restarted the CI run. Cheers!

@hansl
hansl merged commit 36e4d7b into angular:master Jul 5, 2017
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants