Use installing package manager in README #7687
Conversation
ashr81
requested review from
ianschmitz,
iansu,
mrmckeb and
petetnt
as code owners
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| // modifies README.md commands based on user used package manager. | ||
| if (useYarn) { | ||
| try { | ||
| const data = fs.readFileSync(path.join(appPath, 'README.md'), 'utf8'); |
There was a problem hiding this comment.
Rename data to readme and modify that directly instead of creating another formatted variable.
There was a problem hiding this comment.
@iansu I renamed variable to the readme and modifying directly. I used variable naming reference from the same file.
|
This is great. Just one small change from me and then I think we can merge it. @mrmckeb has been doing some work on this part of the code. Any concerns? |
|
@iansu I saw the PR raised by him. I have no concerns. Let me know if you want something to be done from the end. |
|
@iansu can you please review this PR. Is the failing test cases stopping this from getting merged? Or do you have any concerns on the PR? |
|
I think this won't cause any issues for now, I'll need to do a rebase before we move forward on templates anyway. |
|
@mrmckeb cool thanks. If changing the base branch to |
|
Restarting CI |
mrmckeb
changed the title
|
Thank you! |
Fixes #7411
Replaces npm commands to yarn commands if the user uses
yarn