Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added support for javascript config file #126

wants to merge 1 commit into from

Conversation

brunohcastro
Copy link

@brunohcastro brunohcastro commented Aug 10, 2016

#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 Reviewable

@justin808
Copy link
Member

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:

  1. should not use const twice -- that should have gave errors
  2. We need better error handling and messages.
  3. How about if we check the file suffix to match up with js, yaml, yml and that can be in the error handling
  4. Please see https://github.com/shakacode/bootstrap-loader/blob/master/CONTRIBUTING.md
  5. 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

@brunohcastro
Copy link
Author

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.

@justin808
Copy link
Member

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

@justin808
Copy link
Member

@brunohcastro Let's go with my suggestion from the prior comment.

CC: @alexfedoseev

@justin808
Copy link
Member

@brunohcastro Any responses to my comments?

@Judahmeek
Copy link
Contributor

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

@justin808
Copy link
Member

@Judahmeek that would be great! 👍

@justin808
Copy link
Member

@brunohcastro Any interest in getting this one merged? Any interest from other community members?

@justin808
Copy link
Member

@brunohcastro or @Judahmeek should we still do this one?

@Judahmeek
Copy link
Contributor

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

@justin808
Copy link
Member

Let's see if others vote for doing this one.

@justin808
Copy link
Member

@brunohcastro Any compelling reasons to include this?

@justin808 justin808 mentioned this pull request Mar 14, 2017
@justin808
Copy link
Member

Does anybody want this feature?

@morinted
Copy link

morinted commented Jun 9, 2017

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.

@justin808
Copy link
Member

@morinted want to take over this PR and see if this meets your needs?

@justin808
Copy link
Member

@morinted @brunohcastro Should this one get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants