bootstrap-loader
bootstrap-loader copied to clipboard
Added support for javascript config file
#125 Added support for javascript config file by trying to stringify the file in JSON after requiring it with CommonsJS. If the stringify fails for any reason, the original fs.readFileSync
kicks in and process the file in the original way.
The addition supports v1 and v2 AFIK.
Thanks @brunohcastro!
Some comments...
Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.
src/utils/parseConfig.js, line 15 [r1] (raw file):
const configContent = JSON.stringify(require(configPath)); } catch (e) { const configContent = stripComments(fs.readFileSync(configPath, 'utf8'));
@brunohcastro:
- should not use const twice -- that should have gave errors
- We need better error handling and messages.
- How about if we check the file suffix to match up with js, yaml, yml and that can be in the error handling
- Please see https://github.com/shakacode/bootstrap-loader/blob/master/CONTRIBUTING.md
- We need test examples, so that future commits don't break your change. You can amend this section: https://github.com/shakacode/bootstrap-loader/blob/master/CONTRIBUTING.md#testing-changes-to-the-repo
CC: @alexfedoseev
Comments from Reviewable
Hey @justin808 I opened the pull request before I read the contributing doc. I'm just pretty new in colaborating on open source projects. Sorry for that.
My first choice was to change the file extensions and then select which type of require method to use, but I like the .bootstraprc
name for the file. But I have to agree that leaving the possibility to write the same file in three different languages a bit risky.
I will create the tests and error checks and get back to you.
@alexfedoseev What's your take on supporting a Javascript config file? Maybe we'd name it .bootstraprc.js
by default. The advantage of having a .js
suffix is that text editors will probably handle the file.
@brunohcastro Let's go with my suggestion from the prior comment.
CC: @alexfedoseev
@brunohcastro Any responses to my comments?
@justin808 The "brunohcastro wants to merge 1 commit into shakacode:master
from unknown repository
" up at the top suggests that this PR is dead.
However, I'd be happy to work on this if you and @alexfedoseev think it's worthwhile.
@Judahmeek that would be great! 👍
@brunohcastro Any interest in getting this one merged? Any interest from other community members?
@brunohcastro or @Judahmeek should we still do this one?
@justin808 I'd be happy to work on this if you think it's worthwhile. Last we spoke, you mentioned the need to balance additional features against the increase to the code base, so I wasn't sure you were interested in seeing this feature added.
Let's see if others vote for doing this one.
@brunohcastro Any compelling reasons to include this?
Does anybody want this feature?
One thing that I had used in bootstrap-sass-loader was a config file that just imported a dev config and changed some variables to make it fit for production. With the current setup it seems like I need to have two full config files if I want any differences.
@morinted want to take over this PR and see if this meets your needs?
@morinted @brunohcastro Should this one get merged?