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

Issue 1394 adding no bin links option #4036

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions packages/create-react-app/createReactApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const program = new commander.Command(packageJson.name)
'use a non-standard version of react-scripts'
)
.option('--use-npm')
.option('--no-bin-links')
.allowUnknownOption()
.on('--help', () => {
console.log(` Only ${chalk.green('<project-directory>')} is required.`);
Expand Down Expand Up @@ -159,10 +160,11 @@ createApp(
program.verbose,
program.scriptsVersion,
program.useNpm,
program.binLinks,
hiddenProgram.internalTestingTemplate
);

function createApp(name, verbose, version, useNpm, template) {
function createApp(name, verbose, version, useNpm, binLinks, template) {
const root = path.resolve(name);
const appName = path.basename(root);

Expand Down Expand Up @@ -218,7 +220,16 @@ function createApp(name, verbose, version, useNpm, template) {
version = '[email protected]';
}
}
run(root, appName, version, verbose, originalDirectory, template, useYarn);
run(
root,
appName,
version,
verbose,
binLinks,
originalDirectory,
template,
useYarn
);
}

function isYarnAvailable() {
Expand All @@ -235,7 +246,7 @@ function shouldUseYarn(appDir) {
return (mono.isYarnWs && mono.isAppIncluded) || isYarnAvailable();
}

function install(root, useYarn, dependencies, verbose, isOnline) {
function install(root, useYarn, dependencies, verbose, binLinks, isOnline) {
return new Promise((resolve, reject) => {
let command;
let args;
Expand Down Expand Up @@ -275,6 +286,10 @@ function install(root, useYarn, dependencies, verbose, isOnline) {
args.push('--verbose');
}

if (binLinks === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this conditon. Should we not be checking that the binLinks option was passed before adding the argument?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay. I understand now. Thanks for clarifying.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Just let me know if I can help or explain anything else!

args.push('--no-bin-links');
}

const child = spawn(command, args, { stdio: 'inherit' });
child.on('close', code => {
if (code !== 0) {
Expand All @@ -293,6 +308,7 @@ function run(
appName,
version,
verbose,
binLinks,
originalDirectory,
template,
useYarn
Expand All @@ -318,9 +334,14 @@ function run(
);
console.log();

return install(root, useYarn, allDependencies, verbose, isOnline).then(
() => packageName
);
return install(
root,
useYarn,
allDependencies,
verbose,
binLinks,
isOnline
).then(() => packageName);
})
.then(packageName => {
checkNodeVersion(packageName);
Expand Down