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

WIP: Add a 'run' script #245

wants to merge 4 commits into from

Conversation

lacker
Copy link
Contributor

@lacker lacker commented Jul 27, 2016

This adds a run script so that you can invoke a script in the same ES6-and-React-compatible environment you get when you run npm start, but in node.

To run a script:

  • Put your script in src, like at src/foobar.js
  • react-scripts run foobar.js

I wanted to get feedback if this seems like it's headed in the right direction. In particular are these decisions the right behavior:

  • This doesn't compile node_modules.
  • This doesn't set NODE_ENV.
  • This stores the compiled files in a global temp directory.
  • This doesn't automatically polyfill fetch.

Things that I'm pretty sure are not the ideal behavior but can be improved later:

  • This doesn't cache anything
  • No sourcemaps
  • No arguments are passed to the script

It seems like a lot of the behavior will be similar between "run a one-off script" and "run unit tests" so perhaps we should have some underlying library that is shared between this and testing like #216 .

@ghost ghost added the CLA Signed label Jul 27, 2016
// 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.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2016

Can you offer a few use cases of how you’d use this?

@lacker
Copy link
Contributor Author

lacker commented Jul 28, 2016

Some use cases this should be good for:

  • In the React Native website, there's a preprocessing script that converts markdown files to JSON. I would like to convert the RN website to use the create-react-app environment. The preprocessing script could just be in ES5 but then it's annoying that some of the files in your codebase can use ES6+JSX features and some can't. I would like to invoke this preprocessing script via react-scripts run so it can share files with the website.
  • If you use a service like Firebase you often want a script to prepopulate an empty backend. Again, you can just invoke it with node but then part of your codebase is using different language features than others.

@lacker lacker mentioned this pull request Jul 28, 2016
var outputPath = os.tmpDir();
var outputFile = 'entry.js';

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

@lacker lacker changed the title Add a 'run' script WIP: Add a 'run' script Jul 28, 2016
@lacker
Copy link
Contributor Author

lacker commented Jul 28, 2016

Another use case this would be helpful for - the Relay starter kit has a script to automatically generate a graphql file. https://github.com/relayjs/relay-starter-kit/blob/master/scripts/updateSchema.js It would be nice if all the FB open source React examples used the create-react-app format, but that means we need a solution for running these miscellaneous one-off scripts. Currently it uses babel-node but that doesn't work if the .babelrc isn't exposed.

@cpojer
Copy link
Contributor

cpojer commented Jul 29, 2016

We can definitely leverage jest-runtime for this if we want to, which is only a one-line thing and would re-use Jest's config.

@cpojer
Copy link
Contributor

cpojer commented Jul 29, 2016

I thought about this a bit and your goal is to run ES2015+ code in older/current node engines, right? I don't understand why you need a webpack config for that.

Is there a reason why this cannot be as simple as:

scripts/run.js

require('babel-register')(require('../config/babel.dev'));
require(scriptPath);

?

If however your goal for the run script is to actually require and run application code (like "App.js") and we end up merging #250, I'm happy to make some changes to jest-runtime to work with less configuration just like jest-cli. jest-runtime and jest-repl allow you to run application code using node. The idea behind it is that it allows to require modules that use @providesModule from Facebook's haste module system which is something that people in react-native do a lot. If you use this system, you cannot resolve modules using node but instead you have to use something like jest-haste-map and jest-resolve; which is what backs jest and jest-runtime. I'm happy to explain this stuff in more detail but I'm quite convinced it is overkill and a simple solution using babel-register is probably enough.

@lacker
Copy link
Contributor Author

lacker commented Jul 29, 2016

@cpojer The main reason that run is not just as simple as you describe is that we handle importing css and image files in webpack, but not in babel. Initially it seemed like a pain to maintain a no-webpack environment that worked the same way. But, if we're already going to do that for unit testing, maybe there is some relatively simple way to make babel just ignore the css and image stuff, like you have in your Jest PR. And then this could be a lot simpler and skip the webpackisms.

Do you know if there is a babel-based non-jest way to do the css and image stubbing you had in your PR?

@cpojer
Copy link
Contributor

cpojer commented Jul 29, 2016

I still don't understand why you need this. Can you explain what the point
of run is? Arbitrary scripts aren't application code and aren't going to
pull in static resources.

If that is your intention however, I'd propose using jest-runtime after
#250 merges. I can help set it up, it'll be only a few lines of code.
On Sat, Jul 30, 2016 at 05:23 Kevin Lacker notifications@github.com wrote:

@cpojer https://github.com/cpojer The main reason that run is not just
as simple as you describe is that we handle importing css and image files
in webpack, but not in babel. Initially it seemed like a pain to maintain a
no-webpack environment that worked the same way. But, if we're already
going to do that for unit testing, maybe there is some relatively simple
way to make babel just ignore the css and image stuff, like you have in
your Jest PR. And then this could be a lot simpler and skip the webpackisms.

Do you know if there is a babel-based non-jest way to do the css and image
stubbing you had in your PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#245 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA0KGfuYo2bmPyr3N6vVWstwqag27B_ks5qamFRgaJpZM4JWsBi
.

@lacker
Copy link
Contributor Author

lacker commented Jul 30, 2016

There is no real use for running a script that imports a css or image file, at least not with these null stubs. I just thought it is frustrating to have two slightly different ways something can run, and you have files that you can import in one environment but not in others. The real use case for this however is using shared library functions, not shared React components. You might inadvertently be exporting some useful functions in a file you want to require for your script, but it's probably poor style.

OK so I am convinced the webpack-based way is not the way to go. We should probably just start with something like the simple babel wrapper, see if the lack of handling css and image imports is actually a big problem, and we can always enhance the script runtime later. I will close this one and open a new PR

@lacker lacker closed this Jul 30, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants