search

hapipal / confidence

Dynamic, declarative configurations
Other
264 stars 44 forks source link

Issue 70: Document array behavior #94

Closed dpmott closed 4 years ago

dpmott commented 4 years ago

Issue 70: Document array behavior

This addresses https://github.com/hapipal/confidence/issues/70

The original intent of this PR was to simply update the README.md file to include an explanation that arrays are always merged.

Just getting set up in a Windows environment revealed an issue in bin/confidence (annotate() is not a function, and resolving the configuration path in Windows - tests have been added/updated for these).

Then, as I was developing the examples that I would put in the README.md file, I discovered that a base array declared as follows would not be detected as an array, and would be overridden and not merged:

{
  $base: {
    $value: ['array']
  }
}

This led me to investigate how to make that functionality work consistently in all cases, hence the changes in lib/store.js.

After making those straightforward changes, I realized that I had no way to use Confidence for the use case posted in my original issue, which was the need for an array in the base configuration to be overridden by a filtered value.

So, I investigated how to go about adding a flag to the base array, such that the behavior of merging or overriding the array could be controlled from within the definition of the configuration. This led to the addition of the $replace flag and a custom Joi validation extension.

This PR represents a breaking change. And also, it reveals the false assumption and inconsistent behavior that arrays were not always merged to begin with. As such, I would appreciate some guidance. If the maintainer(s) would like a separate PR that just updates the README and some test cases to better demonstrate existing functionality so that it can be merged into the current release without introducing any breaking change, then I will be happy to generate that additional PR.

Thanks!

dpmott commented 4 years ago

@patrickkettner if you have the bandwidth, I'd like to get your feedback on this PR, please and thank you. 😃

devinivy commented 4 years ago

@augnin I took a look at this. My perspective is that we should accept it and publish a new major version, but it's your call of course 👍. If no action is taken in a week or so, I will probably step in and move this along. In the next major version I would recommend at least dropping node v8 (and possibly v10 as well, in order to stay in-step with hapi).

augnin commented 4 years ago

@dpmott thanks for the PR