Move hooks and validation into classes

[mmahalwy]

As seen in the docs:

class Author extends AuthorCodegen {
  constructor(em: EntityManager, opts: AuthorOpts) {
    super(em, opts);
  }
}

authorConfig.beforeFlush(async () => ...);

authorConfig.afterCommit(async () => ...);

At first glance, this feels confusing. As a developer, I'd assume the hooks should be placed on the model/entity. Something like this could make more sense:

class Author extends AuthorCodegen {
  static beforeFlush = [
    async () => ...,
    async () => ...,
  ]
}

Secondly, coming from the rails hook hell, it's pretty easy to have hooks being placed in different parts of the app, causing hook hell and losing track of where changes are being made and what hooks are being made.

[mmahalwy]

In my opinion, this is also the case with validations. Ideally, we either have validations as:

class Author extends AuthorCodgen {
  validation = [
    async (author) => ...,
    async (author) => ...,
  ]
}

Or following the same format as react-final-form and react-hook-form:

class Author extends AuthorCodegen {
  validate = async (author) => {
    const errors = {};
    
    if (!author.firstName) {
      errors.firstName = 'Missing first name';
    }
    
    return errors;
  }
}

[stephenh]

@mmahalwy yeah, we might end up doing this. Fwiw I assert once you get used to it, it really doesn't matter / is not that different in practice from being "inside the class".

Traditionally the reason we've done this is that Joist looks at the populate hints passed to beforeFlush and addRule to setup cross-entity reactivity. I.e. if your validation rule depends on a book's author, the rule will re-run both with the book changes, as well as when the author changes.

And so we need to know "what cross-entity/reactive validation rules does the Author type trigger" at boot time, i.e. before any objects have been instantiated.

So that's why they're top-level module calls, so that Joist can configure the "reactivity graph" before actually loading/creating any instances.

...that said, after we designed the config API around this restriction, we have since started creating a "throw-away" instance of each entity on boot, to detect custom references. So, we could probably use that throw-away instance to observe/scan for static class members as well.

Concretely, your API proposals look fine, but is currently missing a place for the populate/reactivity hints, i.e. it would need to be something like:

class Author extends AuthorCodgen {
  static validation = [
    { hint: { book: "reviews" }, fn: async (author) => ... }
  ]
}

And then the author that is passed to the fn is pre-loaded with the book and reviews, so you can access them synchronously.

[stephenh]

@mmahalwy hm, how would type checking working? I.e. the validation field needs to have a certain shape. I know JS ORMs use this approach, but they don't have to worry about types. I.e.:

class Author {
  static validation = [1,2,3];
}

Needs to be a type error.

Plus we want type hints, like in this example:

class Author extends AuthorCodgen {
  static validation = [
    { hint: { book: "reviews" }, fn: (author) => {
      author.foo <-- to have type checking / auto-complete, the `validation` field needs to be typed like `AuthorValidation` or something like that
    }
  ]
}

We could do something like:

class Author extends AuthorCodegen {
  static validation: AuthorValidation = [
    ...
   ]

Using static in the base class almost percolates down:

type Rule = (fn: string) => boolean;
class Base {
  static validation: Rule[] = [];
}
class Child extends Base {
  static validation = [(s) => s.endsWith("S")];
}

But I get a type error about s: any i.e. the type inference is not working.

Same error w/o `static:

type Rule = (fn: string) => boolean;
class Base {
  validation: Rule[] = [];
}
class Child extends Base {
  validation = [(s) => s.endsWith("S")];
}

Yeah, it needs the explicit type hint:

type Rule = (fn: string) => boolean;
class Base {}
class Child extends Base {
  static validation: Rule[] = [(s) => s.endsWith("s")];
}

Which is not terrible. We already have that issue with custom references and things, but hopefully can clean those up.

[mmahalwy]

@stephenh Would this not work?

class AuthorCodegen {
  static validation: ValidationRule<AuthorCodegen>[];
}

class Author extends AuthorCodegen {
   static validation = [...]
}

[stephenh]

@mmahalwy no, that is this example:

type Rule = (fn: string) => boolean;
class Base {
  static validation: Rule[] = [];
}
class Child extends Base {
  static validation = [(s) => s.endsWith("S")];
}

And unfortunately the s doesn't get the string type applied to it, it's just left as any.

I also tried bumping to the latest TS 4.5 and that didn't help. It seems like in theory TS could do this, but currently is not.

[mmahalwy]

@stephenh hmm yeah that's a bummer :(

[stephenh]

We still sometimes brainstorm about the best place/API for hooks & validation rules, i.e. we happened to talk about it last week for a bit, but the current "just put stuff at the end of the file" has actually been working out pretty well because:

a) it saves a few lines of indentation for each of the lambdas, and
b) it prevents the class from getting huge as hooks & rules add up

So we'll still keep mulling it over, but no explicit plans on changing this atm.