Repository: jquense/yup
Pull request #443 (2019-02-03 02:05:42)

feat: support self references
`key` cannot be a function since [`validateName`](https://github.com/jquense/yup/blob/d02ff5e59e004b4c5189d1b9fc0055cff45c61df/src/Reference.js#L3) ensures that it is a string.

`isSelf` is not used anywhere and `key="."` isn't supported by `property-expr` getter.

For working self references i opened [pull request](https://github.com/jquense/expr/pull/9) in `property-expr` to support empty string path. (currently `getter("")` works, but `getter("", true)` which is used in Reference.js throws)

vonagam: Found usage for `isSelf` field. Such references should not be added to `_deps` and used in `sortFields`.
vonagam: Ok, now i understand why `key` was `"."` for self references...

Made `"."` stand for self references again. Updated stuff to work with such references (thanks to presence of lazy type it wasn't hard).

Only public api change is `Reference.getValue` (don't know if this method considered as public though). New first argument for current value. (Can be made as non-breaking change if this is public api by making it third argument, not first)
vonagam: Three more things:

1) Using self references now it is possible to match joy's signature for `when`:

```js
mixed().when(string(), {then: string().min(5)})
```

Also possible to omit key for self reference:

```js
mixed().when({is: string(), then: string().min(5)})
```

2) In two above examples conditional that was used was not a function or a value but a scheme. This uses `validateSync` of a passed scheme.

3) While writing tests for this stuff i stumbled on problem with `concat`:

```js
yup.number().validateSync([]) // will throw
yup.mixed().concat(yup.number()).validateSync([]) // will return []
```

Even through previous `concat` assigned `_type` to `"number"`, it retained prototype of `mixed`, so no `_typeCheck` from `NumberSchema.prototype` (`ObjectSchema.prototype` has `_cast` and `_validate` redefinitions, so for object it will be even more broken). I fixed it by flipping direction of merging - now target's clone merges into source's clone and source's clone returned.
jquense: Hey thanks for PR it's a bit tough to follow the work here since it seems like it started off as a small code hygiene fix and ended up adding a few features and changing the API. 

Generally it's be great if we can scope PRs to one change, especially when they change the API so it's easier review and other fixes are blocked behind discussion of unrelated changes. 

For this can we clearly state wat you are trying add and why it's needed? That will help me contextualize the change while review ig as well as understand whether API changes are needed
vonagam: My end goal - support `mixed.when()` usage like in joi's api (simple case of example specified in my previous comment) https://github.com/hapijs/joi/blob/v14.3.1/API.md#anywhencondition-options

There are 4 different issues:

1) Ability to create a self reference (reference which returns not sibling at parent but value itself). It seems that it was intended at some point or maybe it was possible before since there is `isSelf` property on `Reference`. This is main obstacle in using `when` like in joi. Self references introduce change in signature of `Reference.getValue()` from `(parent, context)` to `(value, parent, context)`. Depending on whenever you consider this method as public api or not it might be a breaking change (it is not documented anywhere in readme, i assume that only reference constructor is part of public api).

2) Shortcut syntax for using `when` with self references. Since before self references were not existent, there was no need for such shortcuts. With them it is more pleasant to work with plus they are present in joi. Shortcuts do introduce non-breaking changes to signature of `mixed.when()`: single argument usage and when first argument is condition.

3) Ability to use scheme as condition `is` in `mixed.when()`. Currently only constant value or function check is supported.

4) Standalone side mission: fix `mixed.concat()`. I presented example and description of the problem in my last comment (last point). Only this issue can be separated into another PR since it does not depend on existence of self references.

Currently there are 3 commits in this PR so it is simpler to look at changes. 
Commit one addresses 1) problem.
Commit two fixes problem 4).
Commit three implements issues 2) and 3).
vonagam: Additional major refactor of Reference.js file. Changes:

1) Second argument now really is an options object. And it contains `map` function for mapping value from referenced to resolved one. `contextPrefix` option is removed (reason to remove - #423).

2) Allow to look into value deeply, so not only can you use `"."` to get value, but also `".nested.key"` to get a nested value. Just like for context.

3) You can't use key without `$` to get context value. I think it is unexpected behaviour and was not documented. `$` - for context, `.` - for value, empty string - to get parent value, anything else for siblings.

4) Now all possible sources for value are treated equally in a sense that if base for value is absent, then resolved value is `undefined`. Right now context references throw if context is not provided, but do not throw if parent is not provided for siblings. (Partially will close #440)

5) Added `describe()` implementation to `Reference`. Will not throw now if you call `describe()` on a scheme which contains `ref` somewhere.
vonagam: Out of all source code in this pull request right now self references support take only 10 small lines  (other stuff is tests and refactor with features). What are benefits?

1) Joi migration and parity. No need to migrate to `lazy`, if `when` has same functionality.

1) Easier to read, easier (and less) to write than `lazy`. (`lazy` still may win in really complex scenarios though.)

1) `describe()` does not work with `lazy`. Even if it will in the future, it will never will be able to describe its condition or logic. I think `describe()` might find some useful usage down the line.

1) You can't combine `Lazy` but you can have multiply `when`s. Plus schemas with conditions can combine with others schemas.

1) Performance. In [example](https://github.com/jquense/yup#yuplazyvalue-any--schema-lazy) usage for `lazy` a schema created inside `lazy` body, so for complex schemas it might be faster to just concat to two predefined schemas (like in `when`), than to create one from scratch. (Though i might be wrong on this, this is more of a possibility)

1) `mixed.test()` and `Lazy` are powerful tools and may look like an answer to all, but in reality they move burden (of source code, tests, localisation, possible migration and so on) from library to end user. In some cases it is good, but in others (like `describe()`) not only end users suffers, but library suffers too. `Lazy` is a joker, good to have, but if there is an easier and more canonical way to achieve same result, then this way is better.

Personally, this is (plus lack of `alternatives()`) the issue that made me look into the code of yup. Because i am in process of migration from joi (i want to use same validation library for database models, server params and form). And replacing some schemas which use `when` with `Lazy` feels like regression in usability for me.
jquense: Those are good reasons 👍 I will say tho that API parity with `joi` is really not a goal of `yup`, it's never been meant as a drop-in replacement and even if the api was the same, the internal implementation of casting and validations is so different that you'd likely just get different results than the same schema with joi. I appreciate your perspective coming from joi, and we totally want to make that experience good, even if it's a second order concern. 

I agree with you that declarative affordances like `when` provide a nicer experience, i'm also a heavy user of yup, so well aware of the sharp edges. MY concern is primarily in maintaining clear boundaries between the two apis. yes `lazy` is a low level tool, and at some level of complexity i makes sense to say "use that" instead of "add another overload to when". It's good to chat through these things before moving a head.

In this case i think you're right the code is minimal, and the api is nicer, and easy to support. 

Patch:
diff --git a/src/Condition.js b/src/Condition.js
index d1ef6ee..9d03437 100644
--- a/src/Condition.js
+++ b/src/Condition.js
@@ -40,13 +40,9 @@ class Conditional {
     }
   }
 
-  getValue(parent, context) {
-    let values = this.refs.map(r => r.getValue(parent, context));
+  resolve(ctx, options) {
+    let values = this.refs.map(ref => ref.getValue(options));
 
-    return values;
-  }
-
-  resolve(ctx, values) {
     let schema = this.fn.apply(ctx, values.concat(ctx));
 
     if (schema !== undefined && !isSchema(schema))
diff --git a/src/Reference.js b/src/Reference.js
index bcb03b0..f82cead 100644
--- a/src/Reference.js
+++ b/src/Reference.js
@@ -1,48 +1,69 @@
 import { getter } from 'property-expr';
 
-let validateName = d => {
-  if (typeof d !== 'string')
-    throw new TypeError("ref's must be strings, got: " + d);
+const prefixes = {
+  context: '$',
+  value: '.',
 };
 
 export default class Reference {
-  static isRef(value) {
-    return !!(value && (value.__isYupRef || value instanceof Reference));
-  }
+  constructor(key, options = {}) {
+    if (typeof key !== 'string')
+      throw new TypeError('ref must be a string, got: ' + key);
 
-  toString() {
-    return `Ref(${this.key})`;
+    this.key = key.trim();
+
+    if (key === '') throw new TypeError('ref must be a non-empty string');
+
+    this.isContext = this.key[0] === prefixes.context;
+    this.isValue = this.key[0] === prefixes.value;
+    this.isSibling = !this.isContext && !this.isValue;
+
+    let prefix = this.isContext
+      ? prefixes.context
+      : this.isValue
+        ? prefixes.value
+        : '';
+
+    this.path = this.key.slice(prefix.length);
+    this.getter = this.path && getter(this.path, true);
+    this.map = options.map;
   }
 
-  constructor(key, mapFn, options = {}) {
-    validateName(key);
-    let prefix = options.contextPrefix || '$';
+  getValue(options) {
+    let result = this.isContext
+      ? options.context
+      : this.isValue
+        ? options.value
+        : options.parent;
 
-    if (typeof key === 'function') {
-      key = '.';
-    }
+    if (this.getter) result = this.getter(result || {});
 
-    this.key = key.trim();
-    this.prefix = prefix;
-    this.isContext = this.key.indexOf(prefix) === 0;
-    this.isSelf = this.key === '.';
+    if (this.map) result = this.map(result);
+
+    return result;
+  }
 
-    this.path = this.isContext ? this.key.slice(this.prefix.length) : this.key;
-    this._get = getter(this.path, true);
-    this.map = mapFn || (value => value);
+  cast(value, options) {
+    return this.getValue({ ...options, value });
   }
+
   resolve() {
     return this;
   }
 
-  cast(value, { parent, context }) {
-    return this.getValue(parent, context);
+  describe() {
+    return {
+      type: 'ref',
+      key: this.key,
+    };
   }
 
-  getValue(parent, context) {
-    let isContext = this.isContext;
-    let value = this._get(isContext ? context : parent || context || {});
-    return this.map(value);
+  toString() {
+    return `Ref(${this.key})`;
+  }
+
+  static isRef(value) {
+    return value && value.__isYupRef;
   }
 }
 
diff --git a/src/mixed.js b/src/mixed.js
index 540057e..27f781c 100644
--- a/src/mixed.js
+++ b/src/mixed.js
@@ -134,11 +134,10 @@ const proto = (SchemaType.prototype = {
     return !this._typeCheck || this._typeCheck(v);
   },
 
-  resolve({ context, parent }) {
+  resolve(options) {
     if (this._conditions.length) {
       return this._conditions.reduce(
-        (schema, match) =>
-          match.resolve(schema, match.getValue(parent, context)),
+        (schema, condition) => condition.resolve(schema, options),
         this,
       );
     }
@@ -147,7 +146,7 @@ const proto = (SchemaType.prototype = {
   },
 
   cast(value, options = {}) {
-    let resolvedSchema = this.resolve(options);
+    let resolvedSchema = this.resolve({ ...options, value });
     let result = resolvedSchema._cast(value, options);
 
     if (
@@ -240,12 +239,12 @@ const proto = (SchemaType.prototype = {
   },
 
   validate(value, options = {}) {
-    let schema = this.resolve(options);
+    let schema = this.resolve({ ...options, value });
     return schema._validate(value, options);
   },
 
   validateSync(value, options = {}) {
-    let schema = this.resolve(options);
+    let schema = this.resolve({ ...options, value });
     let result, err;
 
     schema
@@ -268,7 +267,7 @@ const proto = (SchemaType.prototype = {
 
   isValidSync(value, options) {
     try {
-      this.validateSync(value, { ...options });
+      this.validateSync(value, options);
       return true;
     } catch (err) {
       if (err.name === 'ValidationError') return false;
@@ -380,11 +379,16 @@ const proto = (SchemaType.prototype = {
   },
 
   when(keys, options) {
+    if (arguments.length === 1) {
+      options = keys;
+      keys = '.';
+    }
+
     var next = this.clone(),
       deps = [].concat(keys).map(key => new Ref(key));
 
     deps.forEach(dep => {
-      if (!dep.isContext) next._deps.push(dep.key);
+      if (dep.isSibling) next._deps.push(dep.key);
     });
 
     next._conditions.push(new Condition(deps, options));
diff --git a/src/object.js b/src/object.js
index f8b3f96..417e83b 100644
--- a/src/object.js
+++ b/src/object.js
@@ -240,7 +240,7 @@ inherits(ObjectSchema, MixedSchema, {
       },
     });
 
-    if (noAllow) next._options.stripUnknown = true;
+    next._options.stripUnknown = noAllow;
 
     return next;
   },
diff --git a/src/util/createValidation.js b/src/util/createValidation.js
index 3cb4c2c..b46f592 100644
--- a/src/util/createValidation.js
+++ b/src/util/createValidation.js
@@ -68,8 +68,10 @@ export default function createValidation(options) {
     ...rest
   }) {
     let parent = options.parent;
-    let resolve = value =>
-      Ref.isRef(value) ? value.getValue(parent, options.context) : value;
+    let resolve = item =>
+      Ref.isRef(item)
+        ? item.getValue({ value, parent, context: options.context })
+        : item;
 
     let createError = createErrorFactory({
       message,
diff --git a/src/util/sortFields.js b/src/util/sortFields.js
index b544d94..6876262 100644
--- a/src/util/sortFields.js
+++ b/src/util/sortFields.js
@@ -23,7 +23,7 @@ export default function sortFields(fields, excludes = []) {
 
       if (!~nodes.indexOf(key)) nodes.push(key);
 
-      if (Ref.isRef(value) && !value.isContext) addNode(value.path, key);
+      if (Ref.isRef(value) && value.isSibling) addNode(value.path, key);
       else if (isSchema(value) && value._deps)
         value._deps.forEach(path => addNode(path, key));
     }


Test patch:
diff --git a/test/mixed.js b/test/mixed.js
index a71efe8..c012857 100644
--- a/test/mixed.js
+++ b/test/mixed.js
@@ -656,7 +656,7 @@ describe('Mixed Types ', () => {
 
   it('should handle multiple conditionals', function() {
     let called = false;
-    let inst = mixed().when(['prop', 'other'], function(prop, other) {
+    let inst = mixed().when(['$prop', '$other'], function(prop, other) {
       other.should.equal(true);
       prop.should.equal(1);
       called = true;
@@ -665,7 +665,7 @@ describe('Mixed Types ', () => {
     inst.cast({}, { context: { prop: 1, other: true } });
     called.should.equal(true);
 
-    inst = mixed().when(['prop', 'other'], {
+    inst = mixed().when(['$prop', '$other'], {
       is: 5,
       then: mixed().required(),
     });
@@ -720,6 +720,36 @@ describe('Mixed Types ', () => {
     inst.default().should.eql({ prop: undefined });
   });
 
+  it('should support self references in conditions', async function() {
+    let inst = number().when('.', {
+      is: value => value > 0,
+      then: number().min(5),
+    });
+
+    await inst
+      .validate(4)
+      .should.be.rejectedWith(ValidationError, /must be greater/);
+
+    await inst.validate(5).should.be.fulfilled();
+
+    await inst.validate(-1).should.be.fulfilled();
+  });
+
+  it('should support conditional single argument as options shortcut', async function() {
+    let inst = number().when({
+      is: value => value > 0,
+      then: number().min(5),
+    });
+
+    await inst
+      .validate(4)
+      .should.be.rejectedWith(ValidationError, /must be greater/);
+
+    await inst.validate(5).should.be.fulfilled();
+
+    await inst.validate(-1).should.be.fulfilled();
+  });
+
   it('should use label in error message', async function() {
     let label = 'Label';
     let inst = object({
diff --git a/test/object.js b/test/object.js
index 543fd39..2069d51 100644
--- a/test/object.js
+++ b/test/object.js
@@ -329,6 +329,17 @@ describe('Object types', () => {
     ]);
   });
 
+  it('should work with noUnknown override', async () => {
+    let inst = object()
+      .shape({
+        prop: mixed(),
+      })
+      .noUnknown()
+      .noUnknown(false);
+
+    await inst.validate({ extra: 'field' }).should.become({ extra: 'field' });
+  });
+
   it('should strip specific fields', () => {
     let inst = object().shape({
       prop: mixed().strip(false),