-
-
Notifications
You must be signed in to change notification settings - Fork 485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for javascript config file #126
base: master
Are you sure you want to change the base?
Added support for javascript config file #126
Conversation
Thanks @brunohcastro! Some comments... Reviewed 1 of 1 files at r1. src/utils/parseConfig.js, line 15 [r1] (raw file):
CC: @alexfedoseev Comments from Reviewable |
Hey @justin808 My first choice was to change the file extensions and then select which type of require method to use, but I like the 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 |
@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 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? |
#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.
This change is