search

react-webpack-generators / react-webpack-template

Simple react webpack template
MIT License
82 stars 43 forks source link

Config -- added base config object for env-independent settings #13

Closed sthzg closed 8 years ago

sthzg commented 9 years ago

This PR updates the app environment configuration. It adds a base.js that may contain env-independent settings. The environment-specific config files (dev.js, test.js and dist.js) merge the base settings, so that the config object imported with import config form 'config'; contains base and env-settings. If settings are defined in the base and env-object, the env object's value takes precedence. Merging utilizes ES2015 Object.assign() which is polyfilled with core-js. The config object returned by the import is now frozen to prevent mutation.

weblogixx commented 8 years ago

@sthzg,

nice one! Do you think the addition of immutability is worth the load of lodash in the frontend? Or did you just include the assign module? Could you provide me with dist sizes with and without the fix? Thank you!

sthzg commented 8 years ago

@weblogixx That's a good point and made me think if it wasn't a better solution to use the ES6/ES2015 Object.assign function:

export default Object.assign({}, base, dev);.

We would have to require a polyfill for Object.assign() (probably from core-js) to work in ES5 environments, but in my opinion that is a better solution in the long term, because the codebase would use ES standards instead of borrowing that particular bit from external libraries like lodash.

Update I read up on Object.freeze. I was under the impression that it has been there forever, but IE7 and IE8 don't support it (compat table). One way around it would be adding an es5-shim behind a conditional comment in index.html (IMO most developers that have to support older IEs will add that or sth. similar anyways). The shim though would only silence the error and not polyfill the freeze. Otherwise freeze could be left out altogether, although I don't like the idea of mutating the config object.

weblogixx commented 8 years ago

Hi @sthzg,

I dont think we should still need to support IE7/8. I would be glad accepting your request with Object.assign with the mentioned polyfill.

sthzg commented 8 years ago

@weblogixx sorry, it took me a bit to get the PR up-to-date. I updated according to the last comment and successfully tested the polyfill on IE11 (no native support for Object.assign()). The dist size of app.js went up from 139kb to 142kb.

weblogixx commented 8 years ago

More than nice! Will merge it on monday. Thank you for your good work.

sthzg commented 8 years ago

:) thanks for merging.