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

feat: finalize resolve() #447

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Feb 4, 2019

This PR allows safely nest lazys and whens in themselves and each other. (Asked in #335 for example)

Currently there is a difference between calling resolve() on lazy and on mixed with whens:

  • lazy.resolve() will return a schema not connected to lazy instance. So calling resolve() on a returned schema does not call lazy body anymore.

  • mixed.resolve() will return a schema resulted by concating base schema and conditional one, as nor mixed.concat() nor mixed.resolve() remove initial when conditions they are present in resulted schema and those conditions get called and merge their results on subsequent resolve calls of merged schema.

So, in a second case it is not a final resolve, but a partial one (resulted schema will contain conditions). For most cases it does not matter (though it cases unnecessary calls and may cause bugs for example with reach).

Here all resolves are made equal and expect to return a non-lazy schema without any conditions remaining. So calling resolve() on a resulted schema will simply return it back. This also allows to make nested conditions and lazy to work without a problem.

It is a breaking change though, since before reach()ed schema's with whens where only partially resolved and expected to run those conditions again. This should be addressed. I think that there are three possible ways:

  • Do not resolve reached schema. And if people need some methods which require resolved schema (like meta()) tell them about resolve(). So making resolve() part of public api.

  • Keep reach behaviour the same, but add new function getAt (or something) which will return non resolved schema. And to tell people to use it now for most cases.

  • Modify signature of reach, introduce flag for resolving or not. (Plus a parent option, since it may be used in top level resolve call.) So maybe reach will become reach(schema, {path, value, parent, context, resolve}).

(Personally i think first one is better, maybe third. Just suggestions.)

Half of proposal is to standardise resolve() behaviour and other half is to use created opportunity to nest things freely.

(This also fixes #381 without #444, as now subsequent resolve() calls to do not cause repeats of concats)

What do you think?

@jquense
Copy link
Owner

jquense commented Feb 4, 2019

Hey thanks again for taking the time to dig into these hairier issues. I'd like to take a step back on this one and frame the problem we are trying to solve at a higher level.

Right now you can't reasonably nest lazy or another when in a when, and vice versa. I do think this is annoying and not obvious. I've always ment to explicitly document that it's not supported. I would counter a bit by saying that nesting is not strictly necessarily. Anything (most?) can be flattened into a single a when or lone lazy call. In my experience using yup that's usually fine, but that would depend on how small and portable one makes schema and app workflows etc.

In terms of reach tradeoffs I think that's a big concern. Form libraries depend on the resolving nature of reach, resolved schema metadata to make form choices, etc. On a functional level tho reach must resolve schema b/c a path may not exist until the schema is resolved.

We would let people deal with that nuance themselves but I would consider users needing to think about resolve explicitly an API failure in terms of mental overhead and complexity.

In terms of implementation I'd love to make resolve more consistent, It was added kinda adhoc with Lazy to try and unify the schema interface so most schema don't need to internally care about whether something is lazy or not.

mixed.resolve() will return a schema resulted by concating base schema and conditional one, as nor mixed.concat() nor mixed.resolve() remove initial when conditions they are present in resulted schema and those conditions get called and merge their results on subsequent resolve calls of merged schema.

Yeah this is annoying, and kinda a result of the overly generalized approach to schema implementation yup uses. I'd love to fix this, I'm just a bit concerned about the implementations I need to dig into how it affects reach and the like, The intent was that resolve finalizes, so there should be a way to deal with this that still handles edge cases.

@jquense
Copy link
Owner

jquense commented Feb 4, 2019

quick specific thoughts:

  • we only need to resolve up to the final returned schema from reach

reach use-cases

  • validating or casting at a path: reach('blah', ...).validate(), which is now mostly covered by validateAt() and friends (which use reach). no need to finalize since validate does right after but doesn't hurt if you do finalize it.

  • Using a nested schema elsewhere object({ foo: reach(other, 'blah') }) I think this is better served by just exporting the nested schema where it's defined, but meh. it's unclear whether this should fully resolve or not, i lean towards it should.

  • schema introspection, i.e. checking meta/describe, etc on a nested path. this should finalize the entire chain.

@vonagam
Copy link
Contributor Author

vonagam commented Feb 4, 2019

Anything (most?) can be flattened into a single a when or lone lazy call.

I think nesting might help with reusability. And that lazy or custom function/helper/test are more imperative way while nesting schemas with when are more of a declarative approach. Plus for describe it is better if things are expressed as standard schemas and not custom functions.

We would let people deal with that nuance themselves but I would consider users needing to think about resolve explicitly an API failure in terms of mental overhead and complexity.

I agree, but i think that there is some inherit complexity and for advanced users, who use lazy and when (especially if they will be nested), it might be better to have some way to say do they want finalisation with reach. (Plus as i mentioned currently reach misses way to pass parent for resolving, so some arguments changes are needed anyway)

validating or casting at a path: reach('blah', ...).validate(), which is now mostly covered by validateAt() and friends (which use reach). no need to finalize since validate does right after but doesn't hurt if you do finalize it.

Assuming that at path 'blah' you have lazy (which can return different types based on a value), i don't really know what do people expect when they do:

reach(schema, 'blah', valueA, contextA).validate(valueB, {context: contextB});

Do they want to validate valueB with types from A finalisation, or do they want schema to still be responsive? (currently in case of lazy it will not be responsive, while in case of whens it will be, but it might cause bugs if types are non-concatable for valueA and valueB)

Using a nested schema elsewhere object({ foo: reach(other, 'blah') }) I think this is better served by just exporting the nested schema where it's defined, but meh. it's unclear whether this should fully resolve or not, i lean towards it should.

Hm... i'm opposite. From a user perspective it is better not to finalise here since if other.blah field depends on some sibling/context or lazy, i expect it to stay the same and react to changes (and especially not to finalise on undefined value and context).

schema introspection, i.e. checking meta/describe, etc on a nested path. this should finalize the entire chain.

Yes and currently this is not supported. It can be implemented by addition of optional {value, parent, context} options object argument. If it is present then before getting data a schema get fully finalised (and if it does not contain lazies or conditions, this option can be safely omitted).

Note: describe will fail here for object if it has lazy or ref as field, since they do not implement describe.

Option: maybe a returned schema from reach will not be finalised but will store options with which it was reached and if it needs to be finalised for a call (like describe), it can use those options at that moment. Sounds like a promising direction for me.

@jquense
Copy link
Owner

jquense commented Feb 4, 2019

reach(schema, 'blah', valueA, contextA).validate(valueB, {context: contextB});

Ideally i think this shouldn't be something they have to think about. again i think not finalizing in that case is the preferable one, it's not not terrible if we do (assuming ppl pas the same context both places which is the common case)

i expect it to stay the same and react to changes (and especially not to finalise on undefined value and context).

I think that would weird depending on the situation, but generally conditions are specific to a broader context, which would have changed if you use it elsewhere. e.g. the target location of the schema may not have the same parent structure or siblings.

const Foo = object({
  other: bool(),
  name: string().when('other', { is: true, then: s => s.required() )
})

const Bar = object({
 name: reach(Foo, 'name', options)
})

I think it would be confusing that name still depended on 'other' existing in the above

Option: maybe a returned schema from reach will not be finalised but will store options with which it was reached and if it needs to be finalised for a call (like describe), it can use those options at that moment. Sounds like a promising direction for me.

That may be a good direction, tho i'm not excited about the idea of schema being stateful in this way, it may end up complicating everything over all in terms of ease of use and understanding.

I think it's fine to start with not resolving the final return value from reach for now, it's always possible to resolve a schema, less so to "unresolve" it.

@vonagam
Copy link
Contributor Author

vonagam commented Feb 4, 2019

I think it's fine to start with not resolving the final return value from reach for now, it's always possible to resolve a schema, less so to "unresolve" it.

I am for not resolving result. That would be a better solution, though a breaking one. Proposal for storing reaching options was made with assumption that you do not want breaking changes yet.

What about describe? Should we add second optional argument for resolving?

And for future: what about parent resolving option? It is not documented in readme for being part of validate options and not present in reach signature. Is it part of public api or is it considered internal only right now? I assume it is needed option if you use reach to get element with possible refs to siblings.

@jquense
Copy link
Owner

jquense commented Feb 4, 2019

[...] assumption that you do not want breaking changes yet.

no, i'm fine with breaking changes as long as we preserve required use-cases.

What about describe? Should we add second optional argument for resolving?

Let's punt on that for right now. this is already inconsistent depending on if you call it after reaching or just on the schema, i'm ok with it being sub optimal but consistent for the near-term.

And for future: what about parent resolving option?

Yeah it's for internal usage right now, which is why it's not documented. We can try and keep it private until it doesn't make sense anymore

Copy link
Owner

@jquense jquense left a comment

Choose a reason for hiding this comment

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

looking great 👍

test/yup.js Outdated Show resolved Hide resolved
src/mixed.js Outdated Show resolved Hide resolved
src/Lazy.js Show resolved Hide resolved
src/Condition.js Outdated Show resolved Hide resolved
test/object.js Outdated Show resolved Hide resolved
@vonagam vonagam force-pushed the feat-final-resolve branch 3 times, most recently from f28d7fd to 178eb5e Compare February 6, 2019 14:10
src/Condition.js Outdated Show resolved Hide resolved
src/Condition.js Outdated Show resolved Hide resolved
@vonagam vonagam force-pushed the feat-final-resolve branch 2 times, most recently from 9047a4f to 189b082 Compare February 7, 2019 19:01
@jquense jquense merged commit afc5119 into jquense:master Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when use when combined with lazy
2 participants