Skip to content

Remove code duplication #1241

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Remove code duplication #1241

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 31 additions & 29 deletions packages/create-react-app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,30 @@
'use strict';

var chalk = require('chalk');
var path = require('path');
var semver = require('semver');

function checkNodeVersion(packageJsonPath) {
var packageJson = require(packageJsonPath);
if (!packageJson.engines || !packageJson.engines.node) {
return;
}

if (!semver.satisfies(process.version, packageJson.engines.node)) {
console.error(
chalk.red(
'You are running Node %s.\n' +
'Create React App requires Node %s or higher. \n' +
'Please update your version of Node.'
),
process.version,
packageJson.engines.node
);
process.exit(1);
}
}

checkNodeVersion(path.resolve(__dirname, 'package.json'));

var currentNodeVersion = process.versions.node
if (currentNodeVersion.split('.')[0] < 4) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this check then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to check the minimal supported version asap for the whole cra to work.
The problem was that it took ~10 minutes with node v0.10 to get to an halt with error, using a lot of resources meanwhile as it still installs all the dependencies, even thou it warns about the problem.

Expand All @@ -54,10 +78,8 @@ if (currentNodeVersion.split('.')[0] < 4) {

var commander = require('commander');
var fs = require('fs-extra');
var path = require('path');
var execSync = require('child_process').execSync;
var spawn = require('cross-spawn');
var semver = require('semver');

var projectName;

Expand Down Expand Up @@ -180,7 +202,13 @@ function run(root, appName, version, verbose, originalDirectory, template) {
process.exit(1);
}

checkNodeVersion(packageName);
var packageJsonPath = path.resolve(
process.cwd(),
'node_modules',
packageName,
'package.json'
);
checkNodeVersion(packageJsonPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure if this line is still useful.
Looks like unreachable code.

Copy link
Contributor

Choose a reason for hiding this comment

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

This lets us increase minimal version with time if react-scripts imposes this but user doesn't update the CLI itself.


var scriptsPath = path.resolve(
process.cwd(),
Expand Down Expand Up @@ -219,32 +247,6 @@ function getPackageName(installPackage) {
return installPackage;
}

function checkNodeVersion(packageName) {
var packageJsonPath = path.resolve(
process.cwd(),
'node_modules',
packageName,
'package.json'
);
var packageJson = require(packageJsonPath);
if (!packageJson.engines || !packageJson.engines.node) {
return;
}

if (!semver.satisfies(process.version, packageJson.engines.node)) {
console.error(
chalk.red(
'You are running Node %s.\n' +
'Create React App requires Node %s or higher. \n' +
'Please update your version of Node.'
),
process.version,
packageJson.engines.node
);
process.exit(1);
}
}

function checkAppName(appName) {
// TODO: there should be a single place that holds the dependencies
var dependencies = ['react', 'react-dom'];
Expand Down