Skip to content
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

feature: support overriding host name on au run, and e2e tests #1132

Closed
wants to merge 58 commits into from

Conversation

shahabganji
Copy link
Contributor

@shahabganji shahabganji commented Jul 1, 2019

This pull request helps you to override the default running host on webpack configuration from localhost to whatever you serve the au run command. This will help you to satisfy both #1122 and #1130

e.g.

au run --host 127.0.0.1 --port 8090 
au run --host 0.0.0.0 --port 8090 

and you can still have the --open command line argument as well to satisfy #1130

au run --host 127.0.0.1 --port 8090 --open

or simply au run --open to run the application on localhost:8080


There was a minor misbehavior with --open on my machine, it opens two tabs, that is also fixed in this PR.

@3cp
Copy link
Member

3cp commented Jul 1, 2019

Please update skeleton/webpack/aurelia_project/tasks/run.json too for the new "host" option. The file is used by au help.

Add help documentation for `host` argument of the `au run` command in run.json file
@shahabganji shahabganji changed the title feature: support overriding host name on au run WIP: feature: support overriding host name on au run Jul 2, 2019
@3cp
Copy link
Member

3cp commented Jul 2, 2019

Looks great. "open" can be removed from cli package.json too.

This makes all bundlers to behave similarly when using webpack
The auto open browser option can now be configured both from aurelia.json file or as an --open flag from au run command
Add "localhost" as the default value in the absence of `host` value in the `aurelia.json` file, or in the `--host` flag of the `au run` command
@shahabganji
Copy link
Contributor Author

@3cp, the CLI bundler part is tested with Alameda, if further testing is required let me know, otherwise remove the WIP flag at the beginning.

If webpack has been selected as the bundler, we can override both port and host

au run --port 8081 --host 127.0.0.1

if CLI bundler has been selected, only port can be overridden, I didn't find how to override the running host, if you know the place, let me know.

au run --port 6060

For the dotnet core template we could override the port via the --port flag, however, for the host it is kinda useless to override it. If you think it should be overridden as well, let me know of a mechanism that I could read the values within .ext files.

@3cp
Copy link
Member

3cp commented Jul 6, 2019

browserSync supports host option too, you can update in the run task.

@shahabganji shahabganji changed the title WIP: feature: support overriding host name on au run, and e2e tests feature: support overriding host name on au run, and e2e tests Jul 26, 2019
@shahabganji
Copy link
Contributor Author

@3cp

karma-coverage > [email protected]: This module is no longer maintained, try this instead:
  npm i nyc
Visit https://istanbul.js.org/integrations for other alternatives.

should we change something for this warning?

@3cp
Copy link
Member

3cp commented Jul 26, 2019

Not in this PR pls :-)

@shahabganji
Copy link
Contributor Author

@3cp,

Which one not in this PR, this commit, or updating istanbul to nyc?

@3cp
Copy link
Member

3cp commented Jul 26, 2019

Yes, I mean outdated istanbul. Replacing it looks like not a small change.

@3cp
Copy link
Member

3cp commented Aug 2, 2019

We have merged and tested the webpack refactoring PR.

The run task has been simplified to a wrapper of npm start.

You can still pass au run --host x.x.x.x --port 8888 to au run, the additional parameters are handled by webpack-dev-server directly.

You would need to rebase on top of current master and resolve the conflicts.

@shahabganji
Copy link
Contributor Author

shahabganji commented Aug 13, 2019

@3cp
How can I gracefully stop the webpack server? programmatically, like the shutdownAppServer we exposed in this PR.

@3cp
Copy link
Member

3cp commented Aug 13, 2019

Probably can do from outside.

I updated release-check to use tree-kill to kill au run (which wraps webpack-dev-server command). It might be useful to you too.

Probably you can use cross-spawn (for reliable win32 spawn) to spawn au run or webpack-dev-server directly, hold the proc instance, then use tree-kill to kill proc.pid.

@3cp
Copy link
Member

3cp commented Aug 13, 2019

Or you can skip au run and webpack-dev-server command, but use webpack-dev-server API. But I remember you pointed out a bug that prevent the dev server API for close to work.

@shahabganji
Copy link
Contributor Author

shahabganji commented Aug 14, 2019

But I remember you pointed out a bug that prevents the dev server API for close to work.

That was for browser-sync.

I am not sure about tree-kill I'll give it a try, but what was weird to me is that when I killed the process with either process.kill(pid) os ps-node 's ps.kill(pid), it looks like they killed the process, but the port is still in use, either unit tests passed or failed.

@3cp
Copy link
Member

3cp commented Aug 14, 2019

but the port is still in use, either unit tests passed or failed

This is what tree-kill solved in release-check. It seems normal kill will skip some child process.

@shahabganji
Copy link
Contributor Author

And one more thing, how could we tell tree-kill to fail the processes with a proper exit-code when tests failed?

@3cp
Copy link
Member

3cp commented Aug 14, 2019

I don't get your question.
tree-kill is for the running dev server process. If dev server failed to start, you should have proc.on('error/exit',cb) to handle it. Only when dev server started properly (by detecting stdout message like release-check task), you start e2e tests, when e2e tests finishes, you tree-kill dev server, and use the result of e2e tests to exit, not the result of tree-kill.

* Update port and host detection with the new mechanism of running webpack applications

* Update shutdownAppServer method to return a Promise which resolves when the app server is down.

* Add host to the aurelia.json
@shahabganji
Copy link
Contributor Author

@3cp
I think it's done. Check it out and let me know of any ideas or feedback.

@3cp
Copy link
Member

3cp commented Aug 15, 2019

Seems you got lots of duplicated commits/changes with those merges. Do you mind to start clean with another branch on top of latest master? New PR is if needed.

@shahabganji
Copy link
Contributor Author

@3cp check out #1143, I close this one in respect of #1143 😄

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

Successfully merging this pull request may close these issues.

3 participants