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

Proposal to improve Validation plugin #94

Closed
parisholley opened this issue Aug 13, 2020 · 16 comments
Closed

Proposal to improve Validation plugin #94

parisholley opened this issue Aug 13, 2020 · 16 comments
Labels
enhancement New feature or request hookstate-4 Marks issues which are solved starting from version 4. plugin An issue affects a plugin

Comments

@parisholley
Copy link
Contributor

I was initially stuck because I wanted my navigation to show badges when "tabs" had validation issues, but didn't want to re-render all the tabs on change. Initially I was doing

state.array.forEach(a => {
  Validation(a.name).validate(a => a.length > 0, 'Name required');
});

The problem with the above though is it will now subscribe my tab/navigation to any validation failures and cause a full re-render of all children.

Once I dug into the code I realized I could write it like this:

Validation(a.array[0].name).validate(a => a.length > 0, 'Name required');

Which actually ends up wildcarding behind the scenes, but not clear from the syntax. At the very least would be helpful to document this current behavior, but I think a more idiomatic syntax would be:

Validation(a.array).forEach(a => {
a.name.validate(a => a.length > 0, 'Name required');
});
@avkonst
Copy link
Owner

avkonst commented Aug 13, 2020

Hi,

I agree that the syntax you propose is good. But before we go further, I I would like to understand the problem you faced. Could you please post a bit more complete example which I could play with and reproduce the issue?

Thanks,
Andrey

@parisholley
Copy link
Contributor Author

my goal is to have tabs, that can validate themselves, and the parent can show error badges based on the children.

const state = useState({});
Validation(a.array1[0].name).validate(a => a.length > 0, 'Name required');
Validation(a.array2[0].name).validate(a => a.length > 0, 'Name required');

return (
<Tabs>
 <Tab error={() => Validation(useState(state).array1).valid()}>
<ManageArray1 state={state} />
</Tab>
 <Tab error={() => Validation(useState(state).array2).valid()}>
<ManageArray2 state={state} />
</Tab>
</Tabs>
);

@avkonst
Copy link
Owner

avkonst commented Aug 13, 2020

What is this one expanded to:

<Tab error={() => Validation(useState(state).array1).valid()}>

Is the useState here invoked within a child component of a Tab component?

@avkonst
Copy link
Owner

avkonst commented Aug 13, 2020

And how do you update the state? Do you change names? Add more items to an array?

@parisholley
Copy link
Contributor Author

yes, useState is invoked in child component which isn't in same render tree as ManageArray, etc

state could be updated in an arbitrary way... for sake of this example, assume i have a form downstream which deletes the name value and causes valid() to equal false

@avkonst
Copy link
Owner

avkonst commented Aug 13, 2020 via email

@parisholley
Copy link
Contributor Author

yes

@avkonst
Copy link
Owner

avkonst commented Aug 17, 2020

Ok. Validation plugin requires greater improvements to leverage API v3. It still uses the style of the v1, when it was originally written. One of the improvements is to allow to define validation rules on attach, for example eg: state.attach(Validation((a) => Validation(a.array1[0].name).validate(a => a.length > 0, 'Name required'))) - in this case large part of the plugin can be simplified (eg. it would not require to track duplicate rules), because validation rules would be defined once on attach but not on every rerender. I will come back to this improvement one day, but now it is not a priority for me.

As a workaround, try to attach the rule for name length within the tab child component, which uses the scoped state. It should reduce touching of the array by a parent component. Can you try this?

@parisholley
Copy link
Contributor Author

realized when digging further, is my [0] method only works if the array is populated, otherwise I get HOOKSTATE-107

see what i can do about re-writing plugin

@parisholley
Copy link
Contributor Author

parisholley commented Aug 23, 2020

gonna work on implementation, but so far, thinking typing would look like this:

  Validation(state).name(name => name && name.length > 0);
  Validation(state.name).validate(name => name && name.length > 0);
  Validation(state.properties).forEach((property) => {
    property.id(id => id && id.length > 0);
    property.values.validate((values => values.length > 0));
    property.values.forEach((value) => {
      value.id(id => id && id.length > 0);
    });
  });

depending on the type of State object you pass in, you'll get a builder back that emulates the state graph

@avkonst
Copy link
Owner

avkonst commented Aug 29, 2020

Hi,

I have been thinking about the plugin your are proposing.

One thing would be good to check is how your plugin adds new validation rules when rerender happens. If a component is rerendered, do you have the same validation rule added twice?

Overall, I understand what you would like to accomplish in code. I do not have major code related comments without trying your plugin with a real app, but I can not do this now as busy with other projects. But I will have an opportunity and needs to validate states in the coming projects for sure, or maybe I will refactor one of my older apps.

So, I suggest the following plan for your contribution:

  1. your validation plugin will go to the new plugin
  2. call it, for example, @hookstate/validation-builder or @hookstate/validation2 (just an idea)
  3. you add new page with a code sample for it to the docs, and list it in the index of plugins in the docs
  4. I publish your plugin to the @hookstate space in NPM
  5. you become a point of contact for this plugin - means if someone asks a questions or adds more changes, you are also involved. ok?

How does it sound?

@parisholley
Copy link
Contributor Author

Yeh, the default behavior would be to re-register (duplicate) the validation, so it is up to the user to move the validation high up the stack where their initial state lives (as the intent is to model your state/validation upfront, which should be consistent regardless of re-render's down stream), or to wrap in useEffect/useMemo/useCallback. Effectively, this plugin is intended for traditional DTO patterns of known models and not on-the-fly validation generation, which is why I'm thinking it may more appropriately be called a Form (or Form Validation) plugin, not "validation".

Doing a de-dupe check would be tough because of the closures. I suppose it is plausible to hash/toString() the closures but I'm not sure how reliable that would be. Maybe the plugin could abstract the call to useMemo(), using the state (or some state property, such as path) as a memo value, and only registering once per state.

As a version one, may just keep it simple an enforce a single-validator per path, and throw an exception if it is already registered so that the user knows they need to handle a re-render case on their end.

1-5 works for me. I'll be pushing this plugin through a production app over the coming weeks, so i'll vet it a bit more before making it an official plugin, but would be good to at least release it in an alpha for people to use if they choose.

@avkonst
Copy link
Owner

avkonst commented Aug 30, 2020

"Effectively, this plugin is intended for traditional DTO patterns of known models and not on-the-fly validation generation"

It means that the validation should be either attached in global space or on plugin attach (see below for more details).

"which is why I'm thinking it may more appropriately be called a Form (or Form Validation) plugin, not "validation""

Form state can be also local state. The philosophy of the Hookstate is not not force a decision for a user which variable to use - local or global.

Doing a de-dupe check would be tough because of the closures. I suppose it is plausible to hash/toString() the closures but I'm not sure how reliable that would be.

The current plugin works like this. I found it is reliable. But better to attach validation upfront once (either global or on attach)

Maybe the plugin could abstract the call to useMemo(), using the state (or some state property, such as path) as a memo value, and only registering once per state.

The plugin does not need useMemo, the core provides the required functionality. There is a callback in the plugin init: https://hookstate.js.org/docs/typedoc-hookstate-core#optional-readonly-init
It receives a state is a first argument, which you might need or might not need to use. But in this callback you can invoke user's defined function to give your plugin state validation model. This is the approach I was hoping to go with in the future. It would look like this:

const state = useState({}).attach(Validation(() => {
    // here user defines and returns state validation rules
}))

I suggest you do this for your plugin. This way it would be possible to use your plugin with local states too (and it would follow the principles of the hookstate philosophy).

In addition to the above (as an optional thing), this approach of "validation model on attach" would allow you do drop proxy builder, because a user can return just a large structure with validation rules. For your example (an array of objects), it would be like:

const state = useState({}).attach(Validation(() => {
    const validators = ((s) => {  /* validator for the root state if any */ }) as ValidationModel<YourRootStateTypeHere> // I can give you the definition of the ValidationModel type, which would allow strong type checking for validation rules at any level, it will mirror the structure of YourRootStateTypeHere
    validators['*'] = (s) => {  /* validator for each element of an array */ }
    validators[0] = (s) => {  /* validator for the 1st element of an array */ }
    validators['*'].name = (s) => {  /* validator for the nested name property of each element of an array */ }
    return validators
}))

This is what I am hoping to come up with in the future. What do you think?

@parisholley
Copy link
Contributor Author

i haven't looked into putting all of the validation into the attach() call, but that seems sensible. i'll try that after i've worked through all of my implementations to make sure there aren't any gotchas.

my issue with going the simple object route is the following

isn't type checked by compiler

while in theory you could require the validation object to follow the State<T> structure, this breaks hard with conditional union types. See my snippet below in how I introduced a whenType function to work around this for interface types in graphql.

it can't intuitively handle conditional fields

This is a big reason why validation state needs to be defined high up the stack, because you still want to validate the object, even if the components which managed substate downstream aren't rendered. That being said, I have tested assigning validation rules in downstream components, you just have to decide on always rendering all subcomponents.

Validation(state.message).required();
Validation(state).whenType('__typename', 'NotificationConfigEmail').subject.required();

complex form dependencies cannot be modeled intuitively in raw json

if you have form fields that are layers deep, and a toggle in the middle of the stack, changes validation behavior in fields downstream in the stack, you need a clean way to only listen to the fields you care about.

const enabled = Validation(state.notifications).when(n => n.enabled.get());
enabled.message.required();
enabled.whenType('__typename', 'NotificationConfigEmail').subject.required();

Validation(state).when(s => s.notificationTimeEnabled.get()).notifications.time.required();

I'm not sure how you could intuitively model when(s => s.notificationTimeEnabled.get() in a simple object pattern as it is pretty easy to find gotchas. Example:

{
  enabled: {
    condition: (value: boolean) => {
      if (value){
         return {
           notifications: {
             message: {
               required: true
             }
           }
         }
      }
    }
  }
}

That could in theory allow you to require a message, if the notification is enabled, but what happens when you need to depend on two different fields?

{
  field1: {
    condition: (value: boolean) => {
      if (value){
         return {
           notifications: {
             message: {
               required: true
             }
           }
         }
      }
    }
  },
  field2: {
    condition: (value: boolean) => {
      if (value){
         return {
           notifications: {
             message: {
               required: true
             }
           }
         }
      }
    }
  }
}

How would the validator behave in that case? Is it suppose to merge? Do we expect both to be true before enabling the require condition? What happens if they return conflicting validations?

I find the fluent API is easier to understand and reason about, and makes dynamic validation a lot easier to work with (especially if you want non-form state to impact which rules are live, simple enough to just wrap validation rules in a if condition)

@parisholley
Copy link
Contributor Author

oh, so one of the reasons I didn't like using the state.attach(Validation) approach, is you can't infer the type of the State when doing that... to make that work, I think there would need to be a ValidationAttach(state, () => {}) method, this way it can infer T in State<T> to give you a type-safe fluent proxy (or simple object if that was an alternative implementation)

@avkonst avkonst changed the title Array Validation Proposal to improve Validation plugin Jan 29, 2021
@avkonst avkonst added enhancement New feature or request plugin An issue affects a plugin labels Jan 29, 2021
@avkonst avkonst added the hookstate-4 Marks issues which are solved starting from version 4. label May 28, 2022
@avkonst
Copy link
Owner

avkonst commented Jul 23, 2022

Hookstate-4 has validation plugin reworked. In combination with the Initializable extension, it is possible to run the code once per created state, for example attach validation rules only once. The ability to combine extensions, add new extension methods and override existing methods gives all the required bits to implement custom variations of the validation extensions. The documentation update will follow soon.

@avkonst avkonst closed this as completed Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hookstate-4 Marks issues which are solved starting from version 4. plugin An issue affects a plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants