bootstrap-loader icon indicating copy to clipboard operation
bootstrap-loader copied to clipboard

Added support for javascript config file

Open brunohcastro opened this issue 8 years ago • 16 comments

#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

brunohcastro avatar Aug 10 '16 19:08 brunohcastro

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

justin808 avatar Aug 10 '16 19:08 justin808

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.

brunohcastro avatar Aug 11 '16 02:08 brunohcastro

@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 avatar Aug 11 '16 19:08 justin808

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

CC: @alexfedoseev

justin808 avatar Aug 21 '16 06:08 justin808

@brunohcastro Any responses to my comments?

justin808 avatar Sep 07 '16 10:09 justin808

@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 avatar Sep 17 '16 05:09 Judahmeek

@Judahmeek that would be great! 👍

justin808 avatar Sep 17 '16 09:09 justin808

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

justin808 avatar Oct 30 '16 23:10 justin808

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

justin808 avatar Nov 16 '16 07:11 justin808

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

Judahmeek avatar Nov 17 '16 16:11 Judahmeek

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

justin808 avatar Nov 18 '16 23:11 justin808

@brunohcastro Any compelling reasons to include this?

justin808 avatar Dec 06 '16 06:12 justin808

Does anybody want this feature?

justin808 avatar May 02 '17 03:05 justin808

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 avatar Jun 09 '17 13:06 morinted

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

justin808 avatar Jun 09 '17 21:06 justin808

@morinted @brunohcastro Should this one get merged?

justin808 avatar Jul 27 '17 21:07 justin808