Add configuration validation and refactor handling of unknown identifiers #612
Conversation
mikabytes
force-pushed
the
176-validate-configuration
branch
from
57c4172 to
24fe4a4
Compare
mikabytes
force-pushed
the
176-validate-configuration
branch
from
24fe4a4 to
b186968
Compare
|
@lencioni If it's convenient for you, would you mind having a look at this code? Or, if you're pressed for time, perhaps I should go ahead and merge it. Should be fairly safe. Thanks |
lib/configurationSchema.js
Outdated
| export function getDefaultConfig() { | ||
| return Object.entries(SCHEMA.properties).reduce((acc, [k, v]) => { | ||
| if (typeof v !== `object` || v === null) { | ||
| throw new Error(`Expected schema key '${k}' to be an object`); |
There was a problem hiding this comment.
Instead of saying "key" I think it might make more sense to say "value at" or something similar. It might also be nice to log typeof v.
There was a problem hiding this comment.
Changed it to
`Expected schema key '${k}' to be an object, but it was of type '${typeof v}'. Got: ${v}`
This error will only happen when we change the defaults in code, so it should be fairly obvious what key has changed, but it's always nice to provide as much information as possible.
There was a problem hiding this comment.
Better but still the wording is a bit off I think. We are talking about the value at that key, not the key itself, but the way it is rded could be understood as referring to the type of the key itself and not its value.
There was a problem hiding this comment.
Ah, I think I understand you better now. Let's give it one more go.
`Expected the value at SCHEMA.${k} to be an object, but it was of type '${typeof v}'. Got: ${v}`
If I change SCHEMA.aliases to 123 on row 14, the resulting error become:
Expected the value at SCHEMA.aliases to be an object, but it was of type 'number'. Got: 123
| // report unknown identifiers the same as we have done in the past | ||
| // so we can showcase that the move to Ajv didn't change previous | ||
| // behaviors |
There was a problem hiding this comment.
I appreciate your attention to detail but I do wonder how important is it to preserve this exact behavior?
There was a problem hiding this comment.
Not important at all 😄 But it helps reduce the scope of this PR and lowers my anxiety of having accidentally introduced some unwanted side-effect.
I was thinking of improving error messages in an upcoming PR.
Faulty identifiers / bad configuration would be very nice to have the information of what the offending value was, and how to fix it. Ajv gives me a lot of duplicate error messages, as well as false negatives when using anyOf, so I think that stuff will need a bit of care anyway.
There was a problem hiding this comment.
Improving error messages sounds great to me. I wonder if anything could be added to the schema that would help with that, or if ajv offers any options to tweak the output it generates out of the box. Or maybe someone else has put out a package that makes ajv errors really nice?
There was a problem hiding this comment.
Good ideas. There must be. I will look into it.
I was also thinking we might move documentation into the JSON schema and auto-generate it. I came across jsonschema2md that might be interesting for this. But that's yet another PR.
|
Thanks for doing this! I reviewed on a phone so sorry about any weird formatting |
|
@lencioni Thanks for your feedback, much appreciated! I put some comments of my own. I'll wait for you to review them, and the changes, before merging this. |
Our configuration schema is big and complex. This is mostly due to every property having the ability to also be a function (dynamic configuration).
Several properties don't belong in configuration. We might want to look into extracting them at a later point. Examples are
cacheLocation,coreModules.. These are also undocumented. They bring in a hairball of dependencies that don't make sense for a schema to deal with. It would clean things up a bit to refactor them out.This implementation will return messages on bad configuration and delete the offending keys. Alternatively, we could crash, but seeing as we have earlier ignored them this seems to be the most reasonable way to go.
Changed one test using Set to use Array instead, as I couldn't validate Sets using
instanceof. Not sure why. But it works.