ariakit: Form onValidation shouldn't throw

Enhancement

Current form onValidation property has two design issues: 1. It throws. 2. It throws non-error.

First, validation method shouldn’t throw it should return a report/explanation why data is invalid. Programmer should decide what to do with this report and probably throw exception. Validation should throw only when something went wrong like invalid options were passed.

Second, throwing non-error object leads is an error-prone behavior due to lack of stack it could lead to hard to track bugs. It seems ok while you control the code within you component, but it still a very bad design.

Motivation

Avoiding this allow to create some unified method, which could be used anywhere outside of the component without a single change.

Possible implementations

It’s better when validation method returns an array of issues, and have the next interface:

type Validator = (any, object?): Array<Issue>

type Issue = {
  path: Array<string|number>
  rule: string
  details: object
}

Simple number validator:

function validateNumber(value:any):Array<Issue> {
  if (typeof value === 'number') {
    return []
  }
  else {
    return [
       {rule: 'isNumber', path: [], details: {expect: true, is: false}}
    ]
  }
}

Such issues could be used with i18n package. Its’ creation could be wordy, but it solvable with factory methods.

You can check how this implemented on practice at TypedProps package.

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 2
Comments: 22 (15 by maintainers)

Most upvoted comments

@diegohaz I mean that validator itself shouldn’t throw its result. The result of validation method call should be returned. And the error should be thrown only when something unexpected happen. But, when you try to send (or use somehow) the data that hasn’t pass the validation, then you should throw.

Throwing non-errors considered as anti-pattern/bad practice anyway. Here are references in eslint and bluebird documentations:

Eslint’s in the first paragraph.
Bluebird’s special paragraph.

rumkin on Nov 8, 2019

I see your point here, but my point was more about versioning. With separate package you can introduce a breaking change into forms (experimetal and opinionated) part and bump major only for this exact package without affecting base package which is tend to be much more stable.

Oh yeah! Right now it’s being “solved” by prefixing the module with unstable_. And I think it’s a matter of how many subjetive decisions we want to make.

With this naming approach, we can say that every new module should start as unstable/experimental, so there’s no hard decision to make here.

But we do have one subjective decision to make: when will this module become stable? For Composite, which will be stable in v1.1.0, this means that it’s being used in production in a large project (WordPress) and all major issues and API decisions were already made and tested.

If we start having modules as separate packages, the subjective decisions we would have to make would increase: should this new module be in a separate package? Will it eventually be integrated into the base library? etc.

And the same question remains: when will this module become stable?

I don’t discard this option (there’s even a @reakit npm scope available if we want to separate the library into more packages in the future). But right now I think separate packages should be used:

When the component is built on top of other dependencies that don’t make sense being in the base library. For example, reakit-playground depends on buble, codemirror and emotion.
When the component API is too different or doesn’t follow what’s described in our basic concepts, with components always rendering only one element and with state hooks exposed as a separate function.

That would be the case, for example, if we come up with a high level component like <Calendar /> that abstracts all the internal components instead of exposing them separately.

I don’t think that’s the case, at least with the current Form API, which still follows the library’s basic concepts. If we want to experiment with completely different APIs, though, I agree that a separate package would be better.

diegohaz on Jun 3, 2020

Why do you think it should be a separate package

Form management may be built on top of the base reakit primitives very differently. Consumer might prefer to use any 3rd party for bunch of reasons (some company standard, api doesn’t fit needs, etc). Reakit may (should) provide own opinionated way to manage forms still, but moving it as separate package allows to have own versioning and continue exploring and experimenting without affecting base library.

It’s not intentional that there’s no Input primitive. There should be one eventually. It’s just not one of my priorities right now, but I will review and accept PRs on it.

Should we start another issue to discuss expected API of Input primitive?

Regarding the API, I tend to prefer throwing an error class like you suggested in the other issue, but I’m all open for suggestions.

Ok, I will use this issue to provide more pros/cons for different options here then.

Guria on Jun 3, 2020