Add option to validate JSON file

[bampakoa]

If the option validate is true and the configuration file is not a valid JSON, it throws an exception. References sindresorhus/electron-store#35

[sindresorhus]

I don't understand why you're wrapping this in try/catch? It has no purpose.

And I think this check would be better done here instead:

conf/index.js

Line 152 in 8ce463d

[bampakoa]

I added the try/catch in order to throw the exception to the consumer of the library so that he can handle it appropriately.

I added the check in the constructor because options object is available there and I thought that it would be a good choice to validate early on.

[sindresorhus]

But you're catching and throwing again. These are identical:

try {
	JSON.parse(fs.readFileSync(this.path));
} catch (ex) {
	throw ex;
}

JSON.parse(fs.readFileSync(this.path));

[sindresorhus]

Sorry about the slow reply. The option needs to be documented too.

[bampakoa]

@sindresorhus this is ready

[sindresorhus]

Thinking about this a bit more, I think it would be better to handle this in https://github.com/sindresorhus/conf/pull/42/files#diff-168726dbe96b3ce427e7fedce31bb0bcR148 Then you also don't have to read the JSON twice.

[bampakoa]

@sindresorhus I set the validate option as this.validate = options.validate; inside the ctor and then inside the setter of the store I check it as:

if (this.validate) {
   JSON.parse(data);
}

but the test does not pass. It gives this.validate as undefined

test('`validate` option', t => {
	fs.writeFileSync(t.context.conf.path, 'Some text');
	const conf = new Conf({cwd: tempy.directory(), validate: true});
	t.throws(() => conf.store);
});

Any ideas?

[sindresorhus]

Hard to say without seeing your code. Can you push what you got?

[bampakoa]

sure, here it is

[sindresorhus]

You cannot use the auto-generated instance here. The .path is different for each instance.

[sindresorhus]

Suggested change

	const conf = new Conf({cwd: tempy.directory(), validate: true});
	const conf = new Conf({cwd: tempy.directory(), validate: true});
	fs.writeFileSync(conf.path, 'Non-JSON data');

[sindresorhus]

Suggested change

	this.validate = options.validate;
	this._validate = options.validate;

It's internal and should be marked as such.

[sindresorhus]

This is incorrect. It will never pass as it's just the same as the parse on line 147.

[bampakoa]

here they are @sindresorhus. Thanks for your suggestions! I also added a second test for valid data.

[sindresorhus]

This should be a t.throws assertion. The whole point of this PR is to throw when invalid config and validate: true.

[bampakoa]

@sindresorhus It will never throw because of this check in line 155:

if (error.name === 'SyntaxError') {
  return plainObject();
}

JSON.parse throws SyntaxError which is already caught. We can either try/catch the JSON.parse in line 143-146 and throw error there, which you have suggested not to do, or remove the check for SyntaxError.

[bampakoa]

@sindresorhus in the last commit, I throw an error with custom message to bypass the caught SyntaxError error later on.

[sindresorhus]

The implementation is still suboptimal and I unfortunately don't have more time to guide you through this.