consider null as a value #317
Conversation
test/rules/any.spec.js
Outdated
| @@ -9,7 +9,7 @@ describe("Test rule: any", () => { | |||
| it("should give back true anyway", () => { | |||
| const check = v.compile({ $$root: true, type: "any" }); | |||
|
|
|||
| expect(check(null)).toEqual([{ type: "required", actual: null, message: "The '' field is required." }]); | |||
| expect(check(null)).toEqual(true); | |||
There was a problem hiding this comment.
It's not good that this case is changed. As you wrote in the PR name null is a value, so the required rule should accept it. It should be true if nullable: false
There was a problem hiding this comment.
Actually I'm not sure I'm following you on this point.
In my mind, the case seems good:
Here we consider that nullable is true by default as it's not explicitly set to false.
Given that any accept any type and with $$root modifier any accept any value, it can be anything but undefined which is not a value.
So when any type is used, null should be a valid value.
At the contrary, if nullable=false is set, any could be anything expect null or undefined.
Unless we consider the default value of nullable to false but I'm scared that it will defeat the purpose and implies bigger changes to the test suite.
There was a problem hiding this comment.
Yeah, you are right, sorry I was wrong, I changed the lines in my mind. By the way, I would like to avoid the breaking change....breaking FV generates breaking changes in Moleculer, it means, we can't use it only just in the next version 0.15
To cover similar issue, we add a global option which switch the old/new behaviour. Can we do something similar here?
There was a problem hiding this comment.
No worries, I also had trouble getting my mind around it at first 😄
I'll add to this PR a global setting to be able to switch between old and new behaviour.
There was a problem hiding this comment.
I've also added some documentation and tests that seems relevant for specific edge cases.
4 cases are tested when considerNullAsAValue option is set to true:
- field is
requiredandnullable,defaultvalue will NOT apply. (default) - field is
requiredbut NOTnullable,defaultvalue will apply if field isnullorundefined. - field is
optionalandnullable,defaultwill NOT apply. - field is
optionalbut NOTnullable,defaultvalue will apply if field isnullorundefined.
There was a problem hiding this comment.
Hi @icebob, feel free to tell me if you need me to do some other changes.
There was a problem hiding this comment.
Thanks, sorry for late, I'm checking...
|
Hi @icebob, is it possible to publish a new release that include this feature ? |
|
Sorry, I forgot it. I've just released as 1.17.0 |
This PR contains breaking changes and should be reviewed with care.
All fields are considered
nullableby default.It allows a field to be:
requiredandnull.optionalbut notnull.fix #296