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: use a schema as when condition #445

Closed
wants to merge 1 commit into from

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Feb 3, 2019

Separate PR from #443.

@jquense
Copy link
Owner

jquense commented Feb 3, 2019

Thanks for sketching this out. I'm not convinced this API is worth the added code, I've gone back and forth a few times on adding this and overall I don't think the caveats and gotchas are probably worth the convenience. The biggest problem is that you need a sync capable schema which is not obvious from the API. And if you do want to use a sync schema it's easy to use one explicitly with the function form of is.

Overall I think trying to keep the core library small is probably more valuable here than the API affordance but let me know what you think

@vonagam
Copy link
Contributor Author

vonagam commented Feb 3, 2019

So, this change requires just 5 lines of code.

Change from:

let isFn =
  typeof is === 'function'	
    ? is
    : (...values) => values.every(value => value === is);

to:

let isFn;

if (typeof is === 'function') {
  isFn = is;
} else if (isSchema(is)) {
  isFn = (...values) => values.every(value => is.isValidSync(value));
} else {
  isFn = (...values) => values.every(value => value === is);
}

Plus i added here makeIsFn function so that we do not create and destroy arrays when we deal with only one value (i assume it is the most popular case). Optional 4 lines.

And the only api change here is that you can now use a sync schema as a condition.

So, i do not see here big costs nor in code increase, nor in api complication.

As for benefits, i think that it is expected that you can use validation schema as a condition (like, they are made for this) without writing custom helper. Plus joi provides such usage, and i see gain in providing same functionality for same named methods (less code change for a user when switching and all joi schemas are sync).

But yes, it does not provide functionality which you currently cannot do. So, while i prefer not to write custom helper for this in my codebase, i am ok with this PR not being merged. Feel free to close without any comments.

@vonagam
Copy link
Contributor Author

vonagam commented Feb 4, 2019

Possible bonus point for this: if is, then and otherwise are schemas then maybe we can use this fact later when implementing describe for schema conditions. (If there will be any way to get describe() without finalisation)

@vonagam vonagam force-pushed the feat-when-is-schema branch from f94498c to f7faff7 Compare February 7, 2019 15:03
@vonagam
Copy link
Contributor Author

vonagam commented Feb 7, 2019

This pull should wait for #447.

So that is.isValidSync(value) could be easily replaced with is.isValidSync(value, options).

@jquense jquense closed this 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.

2 participants