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

WIP: Add a 'run' script #245

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions bin/react-scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ switch (script) {
case 'build':
case 'start':
case 'eject':
case 'run':
spawn(
'node',
[require.resolve('../scripts/' + script)].concat(args),
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"fs-extra": "0.30.0",
"html-webpack-plugin": "2.22.0",
"json-loader": "0.5.4",
"null-loader": "0.1.1",
"opn": "4.0.2",
"postcss-loader": "0.9.1",
"promise": "7.1.1",
Expand Down
1 change: 1 addition & 0 deletions scripts/eject.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ prompt('Are you sure you want to eject? This action is permanent. [y/N]', functi
path.join('config', 'webpack.config.dev.js'),
path.join('config', 'webpack.config.prod.js'),
path.join('scripts', 'build.js'),
path.join('scripts', 'run.js'),
path.join('scripts', 'start.js'),
path.join('scripts', 'openChrome.applescript')
];
Expand Down
2 changes: 1 addition & 1 deletion scripts/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ module.exports = function(appPath, appName, verbose, originalDirectory) {

// Setup the script rules
appPackage.scripts = {};
['start', 'build', 'eject'].forEach(function(command) {
['start', 'build', 'eject', 'run'].forEach(function(command) {
appPackage.scripts[command] = 'react-scripts ' + command;
});

Expand Down
115 changes: 115 additions & 0 deletions scripts/run.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

var fs = require('fs');
var os = require('os');
var path = require('path');
var webpack = require('webpack');

var paths = require('../config/paths');

// Hacky command-line parsing. All flags are just ignored and there
// may be no args.
// TODO: support passing args to the script we're running
var nonflags = process.argv.filter(function(x) { return x[0] !== '-'; });
if (nonflags.length != 3) {
console.error('Usage: react-scripts run foo.js');
console.error(
'foo.js is your script, resolved relative to the src directory.')
process.exit(1);
}
var scriptName = nonflags[2];
var pathToEntry = path.join(paths.appSrc, scriptName);

// Figure out what all the node_modules are.
// We don't want to compile the node_modules, but we still want to
// do a commonjs require for them.
var nodeModules = {};
fs.readdirSync(paths.appNodeModules).filter(function(x) {
Copy link

Choose a reason for hiding this comment

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

This won't cover eg require("lodash/fp/extend") (it'll get bundled instead of marked as an external).
If you want, you could just use webpack-node-externals to handle this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I hadn't tested requires with a / in them. Thanks for pointing me to webpack-node-externals - I'll take a look at that.

return ['.bin'].indexOf(x) === -1;
}).forEach(function(mod) {
nodeModules[mod] = 'commonjs ' + mod;
});

// Construct our script in a temporary file.
var outputPath = os.tmpDir();
Copy link

Choose a reason for hiding this comment

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

FYI: Windows doesn't automatically clean out its temporary directory, and on platforms where /tmp is mounted tmpfs (RAM-backed), this could use additional RAM unnecessarily.

var outputFile = 'entry.js';
Copy link

Choose a reason for hiding this comment

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

Seems kinda weird to call it 'entry.js' when it's technically the output 😉

Copy link

Choose a reason for hiding this comment

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

If I run two scripts at once, would they both be written as eg /tmp/entry.js? That could cause some odd behavior.

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 initially thought it would be in a new temporary directory each time but it looks like I am mistaken on that. Yeah this leads to a race condition & I should fix it.


// Configure webpack.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a specific reason why you chose to write the webpack config inline rather than breaking it out into a separate file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scriptName is different depending on where you choose the entry point to be, so you need a slightly different webpack config for each execution, so you can't just have a webpack config created in its own file like the other webpack configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could do:

// webpack.config.js

module.exports = (scriptName) => {
  return {
    // adapt based on scriptName here
  };
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Is that aesthetically superior to just leaving it all in one file?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has the benefit of matching the new syntax allowed in Webpack 2, where configs can actually be functions that return the config object.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've always grouped them because it makes it easy to sync the different webpack configs. (i.e. have a webpack.base.babel.js that has all the common stuff)

// TODO: support sourcemaps
// TODO: figure out if we want to automatically polyfill fetch
var config = {
entry: [
pathToEntry,
],
target: 'node',
output: {
libraryTarget: 'commonjs2',
path: outputPath,
filename: outputFile
},
externals: nodeModules,
resolve: {
extensions: ['', '.js', '.json'],
},
resolveLoader: {
root: paths.ownNodeModules,
moduleTemplates: ['*-loader']
},
module: {
loaders: [
{
test: /\.js$/,
include: paths.appSrc,
loader: 'babel',
query: require('../config/babel.dev')
},
{
test: /\.css$/,
include: [paths.appSrc, paths.appNodeModules],
loader: 'null'
},
{
test: /\.json$/,
include: [paths.appSrc, paths.appNodeModules],
loader: 'json'
},
{
test: /\.(jpg|png|gif|eot|svg|ttf|woff|woff2)$/,
include: [paths.appSrc, paths.appNodeModules],
loader: 'null',
Copy link

Choose a reason for hiding this comment

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

People probably won't need them, but is there a specific reason to null out all these? What if they're writing a script that processes images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think is the right behavior here? AFAICT there is no standard behavior for requiring an image into a node.js environment, so I just wanted the imports to neither crash nor do anything.

},
{
test: /\.(mp4|webm)$/,
include: [paths.appSrc, paths.appNodeModules],
loader: 'null'
}
]
},
};

// Compile the script
webpack(config).run(function(err, stats) {
if (err) {
console.error('error:', err.code, err.message);
console.error(err);
process.exit(2);
}
if (stats.hasErrors()) {
var errors = stats.toJson().errors;
console.error('stats has', errors.length, 'errors:');
for (var i = 0; i < errors.length; i++) {
console.error(errors[i]);
}
process.exit(3);
}

var scriptPath = path.resolve(outputPath, outputFile);
require(scriptPath);
});