feat: support self references

[vonagam]

key cannot be a function since validateName 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 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:

mixed().when(string(), {then: string().min(5)})

Also possible to omit key for self reference:

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:

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 short syntax it is more pleasant to work with and it is 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. (Can be separated into another PR)

4. Standalone side mission: fix mixed.concat(). I presented example and description of the problem in my last comment (last point). (Can be separated into another PR)

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]

Split PR into 3 parts. This PR consist of two commits now and addresses two first issues each with a test.

[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 - yup.ref(...).transform is not a function #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 Allow undefined context, or a set a default context within the schema #440)

5. Added describe() implementation to Reference. Will not throw now if you call describe() on a scheme which contains ref somewhere.

[jquense]

OK so thinking about it, I don't actually think we should continue to support self references in when. It was support originally as an early hack when lazy didn't exist yet. The reason when continues to exist along side lazy is specifically for when you have interplay between dependencies, i.e. the other fields. As you have seen when conditions are structured and topo-sorted so that values are processed in the right order. For a self reference that's unneeded.

I also think it may be helpful for me to frame the overall goals for the codebase, since you're (very helpfully) jumping in a lot. yup should be "intuitive" and easy to use as well as very expressive, and open up when you need complex behavior. It also should be slim, as a browser first validation library. It's size is the major differentiator between joi (as well as async support but they've kinda caught up there). That's not to say i don't want to add new code, but we should consider new code a net negative to start, and justify it. Often it's worth it but we also want to try and trim we can. I mention this not b/c your adding a lot of bulk (your not) but to give some context for when i push back against additional overloads, or certain convenience API shortcuts. Hopethat doesn't sound off putting. I'm glad to have more input on yup 👍

[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.

2. Easier to read, easier (and less) to write than lazy. (lazy still may win in really complex scenarios though.)

3. 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.

4. You can't combine Lazy but you can have multiply whens. Plus schemas with conditions can combine with others schemas.

5. Performance. In example 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)

6. 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]

this.getter can't be falsey if the parent check is removed

[vonagam]

getter is falsey if path is empty (key = prefix + path). So for key "." path is empty and so there is no getter.

[jquense]

see comment above ☝️

[vonagam]

Now it is:

if (this.getter) result = this.getter(result || {});

Is there any problem with that?

[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.