FormidableLabs / builder

An npm-based task runner
https://github.com/FormidableLabs/builder
MIT License
319 stars 26 forks source link

Feature: Override `ARCHETYPE/webpack/configs/webpack.config.js` with `ROOT/webpack.config.js` #4

Open ryan-roemer opened 9 years ago

ryan-roemer commented 9 years ago

In .builderrc:


---
overrides:
  "builder-react-component/config/webpack/webpack.config.js": "webpack.config.js"

In all of the builder-react-component configs that have a:

var base = require("./webpack.config");

import, we would change to something like:

var base = require(builder.resolve("./webpack.config"));

That infers and switches based on overrides. In this case, ROOT/package.json wouldn't need a scripts override.

exogen commented 9 years ago

To bring the earlier PR discussion here, here's why I think it would be cool if you could override files (webpack.config.js included) without needing to override all the scripts that point to them: it's more granular this way. Here's what I'm guessing would be a very common annoyance if we made you override the scripts as well:

  1. I write my own webpack.config.js just to override a couple things like, say, Babel stage.
  2. I now need to override at least one command (say, build-lib) in scripts as well, not because I want to change any fundamental behavior of the command, but just to point to the new file.
  3. Eventually, the archetype I'm using gets improvements to its default script commands. Like, say, it adds the -p flag to its webpack call.
  4. Unless I'm paying very close attention and know to keep my commands in sync with the archetype, I have no idea that I've now diverged from the archetype far more than intended.

For this reason, I think it should at least be possible for builder/the archetype to use some resolve utility we provide that, for any file path, checks PROJECT for that file first, then the archetype (as if your PROJECT were overlaid/merged with the archetype). You'd still totally have the option to override scripts as well.

ryan-roemer commented 9 years ago

@exogen -- Can you think of a way without having us do Bash-style resolve? Because that significantly increases the complexity of an otherwise simple tool?

I.e., how does builder do this without parsing bash commands? Because of script commands we cannot easily determine "that's a file path!"

ryan-roemer commented 9 years ago

Also, the other issue we'll have to tackle in any scenario: What happens when ROOT/webpack.config.js and archetype diverge?

ryan-roemer commented 9 years ago

Thinking about this more, here's I think what's rubbing me about making builder a much more sophisticated tool. Right now:

It would be great for that to continue because you're not right something that's "specifically builder", your just using builder to control your package.json scripts for a lot of repos. I think that's why efforts:

is seeming like perhaps not the best direction, when we could otherwise keep builder as a (1) dumb, but still (2) quite useful meta aggregation tool.

exogen commented 9 years ago

I think we should just go with the simple & strict design for the initial release – it's forwards-compatible with this feature being considered in the future anyway.

But FWIW, this could be neither magic nor a shell interpreter: builder could do extremely dumb (using "dumb" as a good thing here) substitution on the command before it's run, such that anything starting with $ROOT (or some keyword) e.g. $ROOT/path-to/file gets replaced with whichever path exists in first precedence order (your project, then the archetype). That would be like a 3 line String.replace callback function.

It's not magic because it's being explicitly requested by the archetype's command (using the $ROOT keyword), and it's not parsing anything, just doing dumb substitution (of course there'd then be limits like maybe globs and filenames with spaces, that we generally don't see in scripts anyway).

ryan-roemer commented 9 years ago

I think we should just go with the simple & strict design for the initial release – it's forwards-compatible with this feature being considered in the future anyway.

Yep. Totally.

such that anything starting with $ROOT (or some keyword) e.g. $ROOT/path-to/file

Even that one is tricky, because that breaks down on Windows... And we get into bash interpolation-land (e.g., can you do a regex that actually obeys bash escaping rules?)

Not that there isn't an elegant solution that might arise, but as someone who has spent a lot of time in bash magic and worries about Windows, this is tricky stuff :wink:

curiouslychase commented 9 years ago

Here's the proposed solution I suggested (and the variant that @ryan-roemer pointed out second):

Chase's proposal

(for all configuration files, just using webpack.config.js as an example)

config/webpack/webpack.config.js checks the project ROOT for a webpack.config.js, which would have one extra key (builder) that could have one of two values (extend or override).

builder's webpack.config.js does one of the following branches:

Advantages:

Disadvantages:

Modified proposal

based on @ryan-roemer's interpretation of my suggestion:

"<builderConfigToOverride>": "<rootConfigThatOverrides>"

Advantages:

Disadvantages:

ryan-roemer commented 9 years ago

@chaseadamsio @exogen -- Pairing together your ideas and discussions offline, for a strawman of:

---
overrides:
  "builder-react-component/config/webpack/webpack.config.js": "webpack.config.js"

In all of the builder-react-component that have a:

var base = require("./webpack.config");

Import, we would change to something like:

var base = require(builder.resolve("./webpack.config"));

To handle the overrides passed through .builderrc.

exogen commented 9 years ago

@chaseadamsio I think that example does a good job of "working around" the lack of support for something like this in Builder itself, but we're eventually going to need something more extensible (e.g. how would you do the same with .eslintrc, where we can't put any logic?)

The overrides config looks promising. A couple questions:

  1. How does the webpack command in the archetype's scripts know to perform that substitution? Right now build-dist-min is: webpack --config node_modules/builder-react-component/config/webpack/webpack.config.js – does that change? Does builder do a RegExp replace on the command before it runs? Does it still load the archetype's webpack.config.js but that then checks for the override (effectively doing Chase's suggestion, but more legit)? i.e. you've shown that the webpack dev, test, etc. configs will use builder.resolve, but what about how webpack.config.js itself gets loaded by build-dist-min?
  2. The strawman kind of assumes only files will be overriden (as there are some assumptions being made about the strings being passed to resolve being paths). Is that all we'll ever need to override? i.e. will an archetype ever reasonably want to allow overrides of some other flag like a number, component name, etc...?
exogen commented 9 years ago

^ Answering my own question: thinking it out, if we were to avoid substitution and shell magic, it seems to make some things easier and some harder:

Let's say the build-dist-min command was kept as webpack --config node_modules/builder-react-component/config/webpack/webpack.config.js (how else could we update that path without substitution or shell magic?):

  1. The archetype's webpack.config.js would then need to check for the override. If it exists, it would just return require(OVERRIDE_PATH).
  2. That actually makes things in files like webpack.config.dev.js easier. They could still just import ./webpack.config.js without needing to resolve, and they'd get the overridden version.
  3. But that makes my override my-webpack.config.js harder, because if I try to require the archetype's webpack.config.js to extend it, it's again checking for the override and returning mine, and now I have a loop.

And then we're also back to the situation in Chase's first proposal, where we need logic in the configs themselves and couldn't do this for .eslintrc. So we do need some way to get the overridden path into the scripts commands themselves, before they're run.

curiouslychase commented 9 years ago

@exogen I've been thinking about .eslintrc and it seems like that (so far) is the only configuration that would be a blocker on this, but since .eslintrc can be a javascript file, is it possible that rather than using yaml we could infer if there's a config we can extend from with javascript and extend it in our .eslintrc files?

I'm personally not super keen on the eslintrc-* conventions (or them living in the archetype as the source of truth), simply because I really like that my editor can pick up a directory level config and guide me as I'm working rather than having to run it in the background and check it or run it after I've done all the work.

In bolt I actually scaffold those out for the project and they extend the conventions that live in bolt, so it's the best of both worlds for me...as long as someone's not extending their .eslintrc past the bolt version.

I almost think the convention above should always be the case and the archetypes should always use your root level .eslintrc, but the main reason for that is because of the janky way eslint works with editor plugins more than anything else.

exogen commented 9 years ago

@chaseadamsio That doesn't sound too bad! Especially since we're discussing builder init now which could add them for you.

In general though I'm still pretty weary of adding more than necessary to project root, especially if it references a file in the archetype (like the base .eslintrc) – that requires that the archetype be much more careful about updating its internals. (For example, we've already renamed these boilerplate files .eslintrc-node :arrow_right: .eslintrc-server and .eslintrc-react :arrow_right: .eslintrc-client just in the last few months – we wouldn't have that luxury without bumping semver-major if the "officially blessed" approach would be a bunch of project files digging deep into the archetype).

curiouslychase commented 9 years ago

I'd need to think about it more, but personally, given the choice between the way eslint works in the current bolt archetype (and the builder-react-component) and the .eslintrc in a directory that declares what that directory should have as its eslint config, I prefer the latter just from a developer ergonomic standpoint...the beauty of archetypes though is that I guess it's ultimately up to the maintainer of how they want that archetype to work.