joiful-ts / joiful

TypeScript Declarative Validation for Joi
239 stars 18 forks source link

Feature: Fluent API #19

Closed codeandcats closed 5 years ago

codeandcats commented 5 years ago

Closes #15 by adding support for a fluent API via a Joi decorator as per @laurence-myers's suggestion.

e.g.

class SignUpForm {
    @Joi(joi => joi.string().email().required())
    emailAddress: string = '';

    @Joi(joi => joi.string().min(8).required())
    password: string = '';

    @Joi(joi => joi.string().required())
    firstName: string = '';

    @Joi(joi => joi.string().required())
    lastName: string = '';

    @Joi(joi => joi.number())
    age?: number;
}
  1. Your suggestion was far less work than the initial approach I had in mind using a syntax of @StringSchema().email().required(). However, one feature I was hoping to implement in the fluent API was adding an "autoLabel" method which would give the property a user-friendly label based on the property name, but title-cased (by default, or sentence-cased if desired). This would save having to specify a user-friendly label for each property. Wondering if you had any thoughts on how to achieve the same result with this new syntax?

  2. Do you think Joi is an okay name for this decorator? I was wondering if it should be something a little more semantic, but I had trouble thinking of a better name that was still terse.

Of-course I'm happy to address any feedback you have!

codeandcats commented 5 years ago

I just reread the issue and realised I did not implement the fluent API syntax in the individual schema decorators (StringSchema, NumberSchema, etc). I'll try and get this up soon.

codeandcats commented 5 years ago

FYI, I've just updated the individual schema decorators (specifically AnySchema, ArraySchema, BooleanSchema, DateSchema, FuncSchema, NumberSchema and StringSchema) to have an optional schema builder param like the universal Joi decorator, with the exception that the individual ones are typed to their particular schema.

I've added tests for the schemas that already had basic tests but ran out of steam before adding new test files for the remaining decorators with no coverage at all. I can either add the tests in now or we can cover it as part of this issue.

codeandcats commented 5 years ago

Hey @laurence-myers, I'm keen to hear your opinion on this...

I've been thinking I might have another go at a fluent syntax that doesn't require a callback. It will require more code behind the scenes than the current callback version in this PR, but less code and cleaner syntax for the consumer.

e.g. Current fluent style:

class SignUpForm {
    @Joi(joi => joi.string().email().required())
    emailAddress: string = '';

    @Joi(joi => joi.string().min(8).required())
    password: string = '';

    @Joi(joi => joi.string().required())
    firstName: string = '';

    @Joi(joi => joi.string().required())
    lastName: string = '';

    @Joi(joi => joi.number())
    age?: number;
}

Vs. Proposed fluent style:

type Nillable<T> = T | undefined | null;

class SignUpForm {
    @Joi().email().required()
    emailAddress?: string;

    @Joi().min(8).required()
    password?: string;

    @Joi().required()
    firstName?: string;

    @Joi().required()
    lastName?: string;

    @Joi().number()
    age?: Nillable<number>;
}

Pros:

Cons:

Unless you're strongly opposed to this style, I might give it a go.

laurence-myers commented 5 years ago

Looks great! Nice one. 🎉

I think naming the decorator @Joi() could conflict with importing the joi module, which is usually aliased to Joi. (I think tsdv-joi has a global instance of the joi module called Joi). Can we name it something else, maybe @JoiSchema()? As long as that doesn't conflict with the Joi type definitions. (Naming things is hard, *sigh*...)

Do you think we can make the decorator accept an existing Joi schema definitions? e.g.

const mySchema = Joi.string().max(10);

class Foo {
   @Joi(mySchema)
   someValue: 
}

This could be useful for introducing tsdv-joi to an existing codebase that already uses Joi, allowing re-use of the existing schema.

I would discourage going down the path of the fluent chaining API. It'll double the codebase and required tests, and tsdv-joi already needs better test coverage.

All existing tsdv-joi decorators are actually decorator factories, this might impact your implementation. E.g. instead of a decorator factory @Joi() taking a schema factory function and returning a decorator function, you'll need to implement a decorator factory that preserves state between calls of chained methods, but still returns a decorator function and something that can continue the method chain... I don't know how you can do that.

I don't quite understand how your proposed Joi.labelCase() would work, but I think that should be possible currently with a custom decorator. One drawback of the fluent approach is any new methods need to implemented in tsdv-joi.

codeandcats commented 5 years ago

I think naming the decorator @Joi() could conflict with importing the joi module

Okay, I was really hoping to keep it as terse as possible. I was expecting that most developers would simply import the joi module as joi (in lowercase). I generally use camelCase for module names when importing a module unless the module is a reserved word (like case). But I take your point, especially given joi's own documentation imports it as title case. If it's called JoiSchema that will be inline with all the other top-level constraint decorators. I shall rename it. 🙂

With the implementation of the decorator chaining approach, how I see it working would be that each method in the chain still returns a decorator, but the decorator function also has some methods on it which are the other decorators, letting you chain more and more. Each method in the chain would pass down the state to the next decorator. The state would be something like a list of functions to run which apply the decorator operation to the property. Only the final method in the chain actually gets called as a decorator receiving the target, the property name, etc then goes through the state and applies the operations.

You're right though it does double the tests and would be another part of the library that needs to be kept in sync with joi.

Regarding the label case thing, I actually did create my own decorator in my app code which I've been using. This is what it looks like:

import { getAndUpdateSchema } from 'tsdv-joi/core';

// tslint:disable-next-line:function-name
export function AutoLabel(labelCase: 'title' | 'sentence' = 'title') {
  return (target: any, propertyKey: string | Symbol) => {
    const caseModifier = labelCase === 'title' ? Case.title : Case.sentence;
    const label = caseModifier(String(propertyKey), []);
    getAndUpdateSchema(target, String(propertyKey), schema => schema.label(label));
  };
}

I was just hoping to make it part of the fluent api. But all good. 😄

codeandcats commented 5 years ago

Heya @laurence-myers, I've renamed the Joi decorator to JoiSchema now. 👍

If there's anything else you'd like me to change please let me know, otherwise I think this branch ready to go. Test coverage for added code is at 💯%.

laurence-myers commented 5 years ago

Thanks again for tackling this. I've just merged the type safe decorators into master, could you please resolve the merge conflicts? Then I'll merge this PR.

codeandcats commented 5 years ago

@laurence-myers I've now merged master in and resolved conflicts. Tests passing. 👌

laurence-myers commented 5 years ago

Sorry, you'll need to merge from master again, due to the tslint changes.

codeandcats commented 5 years ago

No worries, I'll merge master in and address conflicts now. 😃

codeandcats commented 5 years ago

Branch is up to date with master now.

laurence-myers commented 5 years ago

Thanks for fixing it up, I'll try to get this merged this weekend.

laurence-myers commented 5 years ago

I was thinking, #38 raised a point that in order to use the @Item() decorator, you need to pass in a Joi schema. For a future task, maybe we can enhance it so it can also accept a function, behaving like @JoiSchema()? So @Items((joi) => joi.string().min(1))