builder icon indicating copy to clipboard operation
builder copied to clipboard

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

Open ryan-roemer opened this issue 8 years ago • 13 comments

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.

ryan-roemer avatar Oct 22 '15 19:10 ryan-roemer

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.

exogen avatar Oct 26 '15 19:10 exogen

@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 avatar Oct 26 '15 19:10 ryan-roemer

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

ryan-roemer avatar Oct 26 '15 19:10 ryan-roemer

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

  • builder run WHATEVER is a drop-in replacement for npm run WHATEVER with an easy migration back and forth.
  • builder concurrent ONE TWO is similarly drop in replaceable with npm run ONE | npm run TWO

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:

  • To make builder a shell interpreter
  • Magically replace / infer file paths

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.

ryan-roemer avatar Oct 26 '15 20:10 ryan-roemer

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

exogen avatar Oct 26 '15 21:10 exogen

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:

ryan-roemer avatar Oct 26 '15 21:10 ryan-roemer

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:

  • project has webpack.config.js? -- no? Just use builder config/webpack/webpack.config.js -- yes? Check for builder key --- builder is extend? Extend the builder's webpack.config.js and opts for the ROOT's values --- builder is override? Just use the ROOT webpack.config.js

Advantages:

  • No extra scripts
  • Easy override
  • No extra configuration on the consumer, it just uses their webpack.config.js

Disadvantages:

  • A little more logic in builder
  • It's kind of magic
  • special key in webpack.config.js for builder

Modified proposal

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

  • Add an overrides key to .builderrc which has:
"<builderConfigToOverride>": "<rootConfigThatOverrides>"

Advantages:

  • No extra logic in webpack.config.js
  • Still no extra scripts tags
  • No special magic in webpack.config.js for builder

Disadvantages:

  • Extra logic in determining which webpack config to use
  • Consumer has to know something about what configs do what

curiouslychase avatar Oct 27 '15 21:10 curiouslychase

@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.

ryan-roemer avatar Oct 27 '15 21:10 ryan-roemer

@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 avatar Oct 27 '15 21:10 exogen

^ 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.

exogen avatar Oct 27 '15 21:10 exogen

@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.

curiouslychase avatar Nov 18 '15 14:11 curiouslychase

@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).

exogen avatar Nov 20 '15 04:11 exogen

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.

curiouslychase avatar Nov 20 '15 04:11 curiouslychase