Issue 1394 adding no bin links option #4036
Conversation
|
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! |
|
The diffs looked fine until your last commit. Please change the line endings back or revert your last commit so we can review this properly. |
tdfranklin
force-pushed
the
issue-1394-adding-no-bin-links-option
branch
from
7884905
to
3ab2e86
Compare
|
@iansu Edit |
| @@ -275,6 +286,10 @@ function install(root, useYarn, dependencies, verbose, isOnline) { | |||
| args.push('--verbose'); | |||
| } | |||
|
|
|||
| if (binLinks === false) { | |||
There was a problem hiding this comment.
I don't understand this conditon. Should we not be checking that the binLinks option was passed before adding the argument?
There was a problem hiding this comment.
Hey @bondz !
Great question! I was trying to follow the same pattern that you use for --use-npm and --verbose, so I was passing the option down the same way that they were. The reason for checking for it to be false is because of the way commander checks args; if any arg is passed with the --no flag, it will set that process to false by default.
So the way it was requested, it looks like the request was to be able to pass an option to create-react-app for --no-bin-links. So if that option is passed, then it will set process.binLinks to false (which is why I specifically checked for it to be false).
There was a problem hiding this comment.
Oh, okay. I understand now. Thanks for clarifying.
There was a problem hiding this comment.
No problem! Just let me know if I can help or explain anything else!
|
What happens if someone specifies |
|
Another great question! And one I was going to mention myself... Unless I'm mistaken, if someone passes --no-bin-links without also passing --use-npm, then nothing will happen as that check will never happen (since it appears to me that yarn is used unless the --use-npm flag is specifically passed). I know the original request is for the ability run the command If you would like me to change it to use NPM if that option is passed, I'll be glad to make that change! |
|
@tdfranklin I suppose if yarn is used, users wouldn't run into the problem caused by bin links in the first place. I'll defer to the maintainers for what the default behaviour should be but this works, thanks. |
|
|
|
@iansu |
|
I looks like it will apply to both currently. It determines which command to use above and then the |
|
@iansu |
Sorry about so many commit's. I forgot to set autocrlf to false (I'm working in Windows) so went back and set that which looks like it changed the whole file to fix the line endings. I'll just note below what lines I actually added code to for your review:
Line 79 - added option for --no-bin-links
Line 163 - passed program.binLinks to createApp()
Line 167 - added binLinks as arg to createApp()
Line 228 - passed binLinks to run()
Line 249 - added binLinks as arg to install()
Lines 289-291 - if statement to push --no-bin-links to npm args
Line 311 - added binLinks as arg to run()
Line 342 - passed binLinks to install()
I added a few console.log's when testing to make sure argument was only being passed when it was called. Running
create-react-app my-appshould have no changes. You would need to runcreate-react-app my-app --use-npm --no-bin-linkswhich should then pass the --no-bin-links command to npm. When I tested those flags, the process completed without any errors.Please let me know if there are any further changes you would like me to make or if I did not do something correctly. Thank you!