Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 70: Document array behavior #94

Merged
merged 4 commits into from
May 14, 2020

Conversation

dpmott
Copy link
Contributor

@dpmott dpmott commented Nov 20, 2019

Issue 70: Document array behavior

This addresses #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
Copy link
Contributor Author

dpmott commented Feb 12, 2020

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

@devinivy devinivy requested a review from augnin April 26, 2020 19:36
@devinivy
Copy link
Member

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

Copy link
Member

@augnin augnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@augnin augnin merged commit 1864196 into hapipal:master May 14, 2020
@augnin
Copy link
Member

augnin commented May 14, 2020

@dpmott thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants