Improvements to frameworks.md and added postinstall script #3674
Conversation
|
I see some changes in |
| "prepublish": "gulp prepublish", | ||
| "pretest": "gulp pretest", | ||
| "start": "node testapp/scripts/web-server.js", | ||
| "test": "node scripts/test.js", | ||
| "install_website":"cd website && npm install", |
There was a problem hiding this comment.
Why would someone outside the core team want to install the website?
| @@ -0,0 +1,35 @@ | |||
| var env = require('./environment'); | |||
There was a problem hiding this comment.
We do not provide first party cucumber support, and the .feature files referenced here do not exist
There was a problem hiding this comment.
I know we do not provide first party cucumber support and there are no feature files, but users who use cucumber would have a look at the frameworks.md file in which we have mentioned a link to a sample cucumber conf.js. we need to either remove it or add a sample conf file. I would suggest let's remove it and i could add a reference link to our protractor-cookbook where users can have a look at the cucumber conf file.
There was a problem hiding this comment.
You could put something in docs/, just make sure it makes clear we don't provide first-party support
There was a problem hiding this comment.
I have added a basic cucumber config file in docs/frameworks.md so that users can refer to it.
| @@ -62,10 +62,12 @@ | |||
| "main": "built/index.js", | |||
| "typings": "built/index.d.ts", | |||
| "scripts": { | |||
| "postinstall": "cd bin && webdriver-manager update && npm run install_website && npm run install_testapp", | |||
There was a problem hiding this comment.
Adding a postinstall like this will cause anyone who used npm install protractor to install the testapp - something almost no one wants and won't even work since the testapp is ignored via .npmignore.
There was a problem hiding this comment.
I am also not a fan of this step. We should avoid postinstall because not everyone is a contributor.
There was a problem hiding this comment.
Yes I get it, users would not need it and not everyone is a contributor, my intention was to simplify things for contributors as its a 3 step npm install. I would remove this!
| cd website | ||
| npm install | ||
| cd .. | ||
|
|
There was a problem hiding this comment.
These instructions should be left in. These instructions are important for contributors.
We could do an npm run install_website here instead.
There was a problem hiding this comment.
This could be done! let me clean up things hear 👍
There was a problem hiding this comment.
Reverted back and removed post_install and I see you have already added npm run install_website !
| npm run install_testapp | ||
|
|
||
| Start that up with | ||
| Start the test application with |
There was a problem hiding this comment.
This is also important to get started for contributing. The user should install the angular 2 dependencies and serve the Angular app.
| @@ -62,10 +62,12 @@ | |||
| "main": "built/index.js", | |||
| "typings": "built/index.d.ts", | |||
| "scripts": { | |||
| "postinstall": "cd bin && webdriver-manager update && npm run install_website && npm run install_testapp", | |||
There was a problem hiding this comment.
I am also not a fan of this step. We should avoid postinstall because not everyone is a contributor.
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
1 similar comment
|
CLAs look good, thanks! |
|
@cnishina can you please review and close this! |
|
LGTM! Thanks @igniteram |
cucumberConf.jsso that it could be referenced in frameworks.md 61ae390, would be happy to send a PR with protractor-cucumber tests for our testapp as well.postinstallscript in package.json as it would help remove the redundancy of manually installing dependencies(website,testapp & webdriver-manager update) for contributors. Updated the same inREADME.mde618754These could be squashed & merged into one commit if needed, let's see what travis has to say 😄 !