Zone.js: Removed PhantomJS Dependency

ossdev07 commented 5 years ago

PhantomJS is an unmaintained project, Removed the phantomjs dependency from the package and added support for chrome headless.

Signed-off-by: ossdev ossdev@puresoftware.com

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ossdev07 commented 5 years ago

Hi @JiaLiPassion

I have added the support of Chrome and ChromeHeadless in karma-base.conf.js file commit id: 2be8e777d2f82de027b6a2957d8acb7b11423d31

Please review and revert.

JiaLiPassion commented 5 years ago

@ossdev07, sorry for the late reply, it seems the CI test failed because can't start chrome, could you check it locally?

ossdev07 commented 5 years ago

Hi @JiaLiPassion

I have added the support for Chrome and ChromeHeadless, and now all the tests are passing on the travis.ci.

I am facing below issue in signing the CLA

You must be an owner of the contributor's group in order to submit this CLA.

I will be glad if you could help me with this

JiaLiPassion commented 5 years ago

@ossdev07, thank you very much, LGTM, one question, could you explain the code here

if (process.arch == "arm64" || process.arch == "aarch64")
{
        browser= 'ChromeHeadless';
}
else{
        browser= 'Chrome';
}

JiaLiPassion commented 5 years ago

@ossdev07, about the CLA, you need to reply I signed it! in the comment. And make sure your email setting in git is the same with your GitHub account.

ossdev07 commented 5 years ago

Thanks for the reply

To launch the Chrome browser we need chrome binary, while For ChromeHeadless we use chromium binary.

In the case of aarch64 architecture, chrome is not supported, while chromium support is available. To support all the architecture, I have added the check.

On Second thoughts it will be better to use chrome headless instead of chrome, here are some of the benefits of using it:

Headless browsers provide automated control of a web page in an environment similar to popular web browsers, but are executed via a command-line interface or using network communication.
Also Headless Chrome is now shipped within Chrome for 10 versions (since 59, now 69).
To provide multi-architecture support for the package it is better to use chrome headless since chrome does not work on aarch64 ( but chrome headless uses chromium which support both x86 and aarch64 ) and travis-ci uses x86 platform to run the test cases.
Using chrome headless instead of chrome will only add multi architecture support to the package hence it is a positive change.

Please do revert what are your thoughts on this

JiaLiPassion commented 5 years ago

@ossdev07, thank you for your explanation, please sign the CLA. And we may merge this PR in the next release.

ossdev07 commented 5 years ago

Sorry to bother you again..!

But, I am facing the issue in signing the CLA https://cla.developers.google.com/clas/new?domain=DOMAIN_GOOGLE&kind=KIND_CORPORATE.

Issue is:

We were unable to find the Google group "ossdev@puresoftware.com". Please make sure that you entered the name of your group correctly and try again.

* required fields

Please bear with me till I found solution regarding this or if you have any inputs regarding this then please let me know.

JiaLiPassion commented 5 years ago

@ossdev07, sorry, I am not sure what is the issue, which field did you input the ossdev@puresoftware.com?

ossdev07 commented 5 years ago

In the field Authorized Contributors Group* I have tried to use ossdev@puresoftware.com as username but it seems that my account is not part of Google groups (due to internal policies).

Although, I am trying to resolve this issue but it may take plenty of time which will result in delay in submission of my changes for this module.

Can you please try to submit the changes by your name and then we can see what lies ahead. This will also unblock the additon of '''Chrome''' support for this module.

Do let me know, if you have any othr idea by which my changes can accepted and used without CLA.

Regards,

JiaLiPassion commented 5 years ago

@ossdev07, I believe Authorized Contributors Group* don't need to input anything, there is a preset value there, here is the page. Screen Shot 2019-03-29 at 15 21 26

But anyway, I don't have the permission to merge the PR, so I will ask other people for help.

ossdev07 commented 5 years ago

@JiaLiPassion Authorized Contributors Group* is one of the mandatory fields, without input receiving the error.

Thanks, it will be a great help, if someone can merge the PR.

JiaLiPassion commented 5 years ago

@ossdev07 , sure, and I believe you can sign as an individual, maybe you can try that. And I will ask other people to look into this one too.

ossdev07 commented 5 years ago

@JiaLiPassion I have signed the CLA as an individual. Please let me know if anything else is required from my side.

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

JiaLiPassion commented 5 years ago

@ossdev07, it is all good, thank you!

ossdev07 commented 5 years ago

@JiaLiPassion When can I expect the changes to be merged with the master branch?

JiaLiPassion commented 5 years ago

@ossdev07, thanks for the waiting, we are working on to rewrite zone.js build system with bazel, so this PR will be merged soon after that PR is ready.

ossdev07 commented 5 years ago

@JiaLiPassion Thank you for letting us know. I hope to see the package with the changes soon. :smiley:

rhenwood-arm commented 5 years ago

Is there an update on this PR? I appreciate it may not be the most important PR, but it would be good to see the removal of PhantomJS as PhantomJS isn't maintained and is unavailable on AArch64 (which I care most about).

JiaLiPassion commented 5 years ago

@rhenwood-arm, sure, this PR is ready to merge, I will try to merge this one with other urgent PR asap.

JiaLiPassion commented 5 years ago

@rhenwood-arm, sorry this PR takes so long, because we are working on the related stuff (The bazel migration), now all the build is working with bazel, so all phantomJS related stuff already gone automatically. Thanks for the contribution and the great work!

alexeagle commented 5 years ago

Thanks @rhenwood-arm - closing since this repo is now archived

angular / zone.js