static-dev / spike-core

:warning: UNMAINTAINED :warning: A modern static build tool, powered by webpack
https://spike.js.org
Other
58 stars 9 forks source link

Allow custom options #134

Closed jescalan closed 8 years ago

jescalan commented 8 years ago

Right now, any options that are passed in to the config but that are not explicitly handled do not make it to the final webpack config object. For example, if you were using a loader like the less loader or something that used the less key on the webpack config object, it would not work, since we have a protective layer over the webpack config object (which we did for a good reason).

Before we ship anything stable we need a solution to this. The issue is that adding arbitrary properties to the webpack config object is not really a good practice nor is it a good API for options, when it comes down to it, since they are mixed in with a variety of essential webpack config keys. For example, what if there was a loader for a language called context that accepted functions? Adding the context key to the webpack object would override an essential config value and completely destroy the functionality of the compile, while being entirely confusing to users.

It also creates a namespace issue. For example, we have a jade client loader and a jade static loader, and they both take options from the jade key, which coincidentally the 'webpack official' jade loader does not use since it takes all options via querystring (no, you cannot pass a function as a local to it, surprisingly, and nobody has complained about this). This is just icky and is not a good design pattern at all. It's not our fault, it's the fault of an API that has not been well-thought-out and is kind of cobbled together, waiting for a disaster to happen.

However, we have an opportunity to wrap and therefore improve on that API. Just want to open up this discussion to see if anyone has thoughts on the best way to do this! The end result should allow users to add properties to the webpack config object directly only when needed , and safely.

My initial thoughts are that we should at very least start by taking stock of the webpack config options that are documented and should not be overridden, and protect those entirely. We also need to put extra protection on certain options that one could use with webpack, but we need to control with spike so that spike can operate correctly. I also would love if the loaders and options you are passing to those loaders could be in the same place, since that just makes sense unlike the current API.

kylemac commented 8 years ago

hmm, i don't really see how this doesn't get ugly.

fwiw, and it doesn't fit with our clean API, but i've seen this problem be solved (kind of) with this project - https://github.com/lewie9021/webpack-configurator

jescalan commented 8 years ago

Ok so here's my thoughts so far after tinkering with this for a bit.

There are certain keys that we have to disallow the user from setting. For example, output, plugins, and resolveLoader. We set these manually (sometimes with a piece customizable via otherwise named options), and they are essential in order for spike to operate correctly. I'll refer to these as disallowed options.

There are another set of keys that are custom and specific to spike, but hold no value sitting on webpack's config object. Most of these options are on config.spike, like the env, locals, matchers, etc. These options we need to collect but do not want to be copied over to the webpack config object. I'll refer to these as noCopy options.

Any other option that is passed in through app.js, I'm of the opinion that we should copy it straight on to the webpack config object. While it will probably be possible to break your site by adding weird config options, by preventing the disallowed options, we at least eliminate most of the major paths towards this. And anything in the list of noCopy keys, we can just copy into config.spike, so that there are no options that are not accessible by plugins either directly or through config.spike.

This isn't a huge reorganization, and doesn't mess with their API much. We could probably do more and make it nicer, but I figure this is at least a first step!