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.

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

Jump to bottom

WIP prompt if port is already in use. #101

Closed
wants to merge 14 commits into from
Closed

WIP prompt if port is already in use. #101

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
552b988
WIP notify if port is used and ask to confirm to use another avaiable…
chocnut Jul 22, 2016
10c6adf
remove env port declaration
chocnut Jul 22, 2016
f0a0f68
rebase and fix port variable declaration
chocnut Jul 23, 2016
2252344
Merge branch 'master' into port-option-for-npm-start
chocnut Jul 24, 2016
ac6d9d2
fix function declarion styles
chocnut Jul 24, 2016
aa40a96
update for function declartion style
chocnut Jul 24, 2016
ea71a78
fix eslint warnings
chocnut Jul 24, 2016
63b4290
revert back since arrow functions are supported
chocnut Jul 24, 2016
c1deb6a
Merge branch 'master' into port-option-for-npm-start
chocnut Jul 25, 2016
c0f391a
Refactor and code cleanup
chocnut Jul 25, 2016
245676c
Port error msg
jlbruno Jul 25, 2016
418e62a
Merge pull request #1 from jlbruno/patch-1
chocnut Jul 26, 2016
78c4bf5
added prompt utility. adress pr comments
chocnut Jul 26, 2016
5cf40d5
rebase from upstream. fix eject merge conflicts
chocnut Jul 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -41,6 +41,7 @@
"chalk": "1.1.3",
"cross-spawn": "4.0.0",
"css-loader": "0.23.1",
"detect-port": "^0.1.4",
"eslint": "3.1.1",
"eslint-loader": "1.4.1",
"eslint-plugin-import": "1.10.3",
Expand Down
160 changes: 96 additions & 64 deletions scripts/start.js
View file
Open in desktop
Expand Up @@ -16,6 +16,9 @@ var WebpackDevServer = require('webpack-dev-server');
var config = require('../config/webpack.config.dev');
var execSync = require('child_process').execSync;
var opn = require('opn');
var detect = require('detect-port');
var readline = require('readline');
var PORT = 3000;

// TODO: hide this behind a flag and eliminate dead code on eject.
// This shouldn't be exposed to the user.
Expand Down Expand Up @@ -66,62 +69,89 @@ function clearConsole() {
}

var compiler = webpack(config, handleCompile);
compiler.plugin('invalid', function () {
clearConsole();
console.log('Compiling...');
});
compiler.plugin('done', function (stats) {
clearConsole();
var hasErrors = stats.hasErrors();
var hasWarnings = stats.hasWarnings();
if (!hasErrors && !hasWarnings) {
console.log(chalk.green('Compiled successfully!'));
console.log();
console.log('The app is running at http://localhost:3000/');
console.log();
return;
}
detect(PORT, function(error, _port) {

var json = stats.toJson();
var formattedErrors = json.errors.map(message =>
'Error in ' + formatMessage(message)
);
var formattedWarnings = json.warnings.map(message =>
'Warning in ' + formatMessage(message)
);
if (PORT !== _port) {
var rl = readline.createInterface({
input: process.stdin,
output: process.stdout
});

if (hasErrors) {
console.log(chalk.red('Failed to compile.'));
console.log();
if (formattedErrors.some(isLikelyASyntaxError)) {
// If there are any syntax errors, show just them.
// This prevents a confusing ESLint parsing error
// preceding a much more useful Babel syntax error.
formattedErrors = formattedErrors.filter(isLikelyASyntaxError);
}
formattedErrors.forEach(message => {
console.log(message);
console.log();
rl.question('Something is already running at http://localhost:' + PORT + '. Would you like to run the app at another port instead? [Y/n] ', function(answer){
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, can you add a space between ) and {: function(answer) {

if(answer === 'Y') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: need space after if

Copy link
Contributor

Choose a reason for hiding this comment

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

Y being capital means it’s the default answer. So specifying nothing, or specifying y, yes, Y, or YES should all be treated as y. Specifying n, N, no, or NO should all be treated as no.

You can see an example of this here. The only difference is that there, no is the default, so we look for explicit yes. Here, it’s the other way around.

PORT = _port;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this gets reassigned, it isn’t a constant, so it shouldn’t LOOK_LIKE_A_CONSTANT.
In general, I don’t think we even need to reassign it.
Let’s keep DEFAULT_PORT a constant, but pass a port argument both to setupCompiler and runDevServer.

config.entry = config.entry.map(function(c){ return c.replace(/(\d+)/g, PORT) }); // Replace the port in webpack config
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nit here function(c) { 😘

compiler = webpack(config, handleCompile);
setupCompiler();
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s bad that we repeat this code in this and the other branch below. This makes it easy to change something in one place and forget to change it in the other one.

Instead, let’s split the logic into something like

function choosePort() {
  return new Promise(resolve => {
    detect(...)
  })
}

and then do something like

choosePort().then(port => {
  setupCompiler(port);
  ...
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it looks like detect() already returns a Promise so let’s just use that.

runDevServer();
}
rl.close();
});
// If errors exist, ignore warnings.
return;
} else {
runDevServer();
setupCompiler();
}
});

if (hasWarnings) {
console.log(chalk.yellow('Compiled with warnings.'));
console.log();
formattedWarnings.forEach(message => {
console.log(message);
function setupCompiler() {
compiler.plugin('invalid', function () {
clearConsole();
console.log('Compiling...');
});

compiler.plugin('done', function (stats) {
clearConsole();
var hasErrors = stats.hasErrors();
var hasWarnings = stats.hasWarnings();
if (!hasErrors && !hasWarnings) {
console.log(chalk.green('Compiled successfully!'));
console.log();
});
console.log('The app is running at http://localhost:' + PORT + '/');
console.log();
return;
}

console.log('You may use special comments to disable some warnings.');
console.log('Use ' + chalk.yellow('// eslint-disable-next-line') + ' to ignore the next line.');
console.log('Use ' + chalk.yellow('/* eslint-disable */') + ' to ignore all warnings in a file.');
}
});
var json = stats.toJson();
var formattedErrors = json.errors.map(message =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're using arrow functions and standard functions in the same file? I don't mind either syntax but I think consistency is a good idea. That is, unless we need the syntax difference for the different ways they handle things, but if it's just arbitrary I'd prefer we just stuck with one :)

'Error in ' + formatMessage(message)
);
var formattedWarnings = json.warnings.map(message =>
'Warning in ' + formatMessage(message)
);

if (hasErrors) {
console.log(chalk.red('Failed to compile.'));
console.log();
if (formattedErrors.some(isLikelyASyntaxError)) {
// If there are any syntax errors, show just them.
// This prevents a confusing ESLint parsing error
// preceding a much more useful Babel syntax error.
formattedErrors = formattedErrors.filter(isLikelyASyntaxError);
}
formattedErrors.forEach(message => {
console.log(message);
console.log();
});
// If errors exist, ignore warnings.
return;
}

if (hasWarnings) {
console.log(chalk.yellow('Compiled with warnings.'));
console.log();
formattedWarnings.forEach(message => {
console.log(message);
console.log();
});

console.log('You may use special comments to disable some warnings.');
console.log('Use ' + chalk.yellow('// eslint-disable-next-line') + ' to ignore the next line.');
console.log('Use ' + chalk.yellow('/* eslint-disable */') + ' to ignore all warnings in a file.');
}
});
}

function openBrowser() {
function openBrowser(port) {
if (process.platform === 'darwin') {
try {
// Try our best to reuse existing tab
Expand All @@ -130,7 +160,7 @@ function openBrowser() {
execSync(
'osascript ' +
path.resolve(__dirname, './openChrome.applescript') +
' http://localhost:3000/'
' http://localhost:' + port + '/'
);
return;
} catch (err) {
Expand All @@ -139,21 +169,23 @@ function openBrowser() {
}
// Fallback to opn
// (It will always open new tab)
opn('http://localhost:3000/');
opn('http://localhost:' + port + '/');
}

new WebpackDevServer(compiler, {
historyApiFallback: true,
hot: true, // Note: only CSS is currently hot reloaded
publicPath: config.output.publicPath,
quiet: true
}).listen(3000, function (err, result) {
if (err) {
return console.log(err);
}
function runDevServer() {
new WebpackDevServer(compiler, {
historyApiFallback: true,
hot: true, // Note: only CSS is currently hot reloaded
publicPath: config.output.publicPath,
quiet: true
}).listen(PORT, 'localhost', function (err, result) {
if (err) {
return console.log(err);
}

clearConsole();
console.log(chalk.cyan('Starting the development server...'));
console.log();
openBrowser();
});
clearConsole();
console.log(chalk.cyan('Starting the development server...'));
console.log();
openBrowser(PORT);
});
}