Add support for configuration files with ".jsonc" extension

[sstephant]

Description of the Change

Adds support for ".mocharc.jsonc" as an alternative name to imply JSON with comments.

Alternate Designs

Add JSON5 support.

Why should this be in core?

Minor enhancement of existing capability already in core.

Benefits

Support for ".jsonc" file extension

Possible Drawbacks

None

Applicable issues

Fixes #3753
semver-minor

[jsf-clabot]

All committers have signed the CLA.

[sstephant]

Note: I suppose the user documentation should be updated too to reflect the changes but I'm not sure where to do it.

[coveralls]

Coverage increased (+0.03%) to 91.684% when pulling fdf7375 on sstephant:issue/3753 into 37febb6 on mochajs:master.

[sstephant]

Note: I suppose the user documentation should be updated too to reflect the changes but I'm not sure where to do it.

I think I found the appropriate location and updated the user documentation in the last commit.

[plroebuck]

Color me silly, but my read on the original issue was simply to add another file extension to the set of Mocha configuration files... where did JSON5 come in?

[boneskull]

@sstephant Thank you for taking this! Looks good, except prefer not to support JSON5 at this time. @plroebuck also had some requested changes.

[sstephant]

Color me silly, but my read on the original issue was simply to add another file extension to the set of Mocha configuration files... where did JSON5 come in?

🤦‍♂️. This is my mistake, I read the issue too fast. I understood JSON with comments support had to be added for files with a .jsonc extension. It's so unusual to see actual JSON files with comments I did not realize mocha was already stripping comments from JSON config files. In my opinion the main purpose of JSON5 is precisely the support of comments in JSON/JS like documents, that's why I introduced the json5 parser.

[plroebuck]

Somewhat related -- asking for opinions here... not changes atm.

Think this is better?

exports.loadConfig = filepath => {
  let config = {};
  const ext = path.extname(filepath);
  try {
    if (/\.ya?ml/.test(ext)) {
      config = parsers.yaml(filepath);
    } else if (ext === '.js') {
      config = parsers.js(filepath);
    } else if (/\.jsonc?/.test(ext)) {
      config = parsers.json(filepath);
    } else {
      throw new Error('no parser for this type file');
    }
  } catch (err) {
    throw new Error(`failed to parse "${filepath}": ${err}`);
  }
  return config;
};

Rationale: However stupid, if I used --config with a Word document argument, the code would throw a SyntaxError (as result of failing JSON.parse) which is utterly unhelpful and its error message doesn't address the actual problem.

[sstephant]

I agree the fallback to the JSON parser can lead to an unhelpful error message.
But in your snippet the inner throw would be hidden by the upper catch was that your intention?
Another point I noticed is that the file extension is case sensitive. Was that on purpose?

[plroebuck]

Threw from try just to pass message to catch where it can generate higher-level Error.

[plroebuck]

Another point I noticed is that the file extension is case sensitive. Was that on purpose?

We support lowercase file extensions.

[plroebuck]

It's so unusual to see actual JSON files with comments I did not realize mocha was already stripping comments from JSON config files.

Yeah, wasn't crazy about using JSON files containing comments (since they won't parse naturally without stripping). Actually prefer the new file extension ".jsonc" since it at least implies something different.

[sstephant]

Yeah, wasn't crazy about using JSON files containing comments (since they won't parse naturally without stripping). Actually prefer the new file extension ".jsonc" since it at least implies something different.

Does that mean comment stripping should be disabled on ".json" file from now on? I didn't do it because it's a breaking change.

[plroebuck]

Does that mean comment stripping should be disabled on ".json" file from now on? I didn't do it because it's a breaking change.

No. Too late now... Pandora's box already opened.

I would be okay with reversing these as below, but not required:

- **JSON**: Create a `.mocharc.jsonc` (or `.mocharc.json`) in your ...

[plroebuck]

@boneskull ping. LGTM

[boneskull]

This is pretty much exactly what I wanted, thank you!

[Alhadis]

You guys might wanna consider exposing a list of supported file extensions (or even just a predicate function which tells if a filepath is a recognised config file).

This can matter for editor plugins when a subset of files are passed to Mocha to prevent it scanning the entire workspace (or if implementation details prevent them giving Mocha open-slather access to the filesystem).

Case-in-point, I very recently upgraded Atom-Mocha to use Mocha 6's new configs feature, and had to work off the existing kruft of predetermined filenames. I can already see this happening in Emacs too...)

EDIT: I did notice exports.CONFIG_FILES in lib/cli/config.js, but its JSDoc tag identifies it as @private, so I don't want to try my chances on that...

[boneskull]

@Alhadis Why does an editor plugin need to scan the entire workspace?

Feel free to send a PR that exposes this information in a convenient manner.

[Alhadis]

@boneskull In Atom's case, the editor's test-runner API is invoked with a list of paths, which may be directories or files. There's no guarantee they'll fall within the same path, and because a translation layer is needed, it isn't as simple as just throwing a bunch of paths at Mocha and expecting it to do The Right Thing™... (it's convoluted, I know).

Will send a PR soon-ish. :)