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

WIP: Fluent Validation Plugin #97

Closed
wants to merge 11 commits into from
Closed

Conversation

parisholley
Copy link
Contributor

@parisholley parisholley commented Aug 23, 2020

Potential implementation direction as a solution to #94 and maybe introduce a new Fluent API abstraction.

Problem:

  • Need a way to support running validation in child components yet maintaining isValid() state in a parent component. Example, multiple tabs with a "error badge" if any of the state displayed within the tab is invalid (and doing so in a way that doesn't cause a full re-render).
  • Need a way to validate data within an array whose length may change at any time.

Proposed API syntax:

    ValidationAttach(state, (validator) => {
      // conditional validation
      const enabled = validator.notifications.when(n => n.enabled.get());
      enabled.message.required();

      // union type support (graphql example), conditional reuse
      enabled.whenType('__typename', 'NotificationConfigEmail').subject.required();
      enabled.whenType('__typename', 'NotificationConfigSlack').channel.required();

      // nested array path support
      validator.roles.name.validate(name => name && name.length > 0);

      // inject any other parts of the state into conditionals
      validator.properties.conditions.when((c, properties) => {
        if (!c.propertyId.get()) {
          return false;
        }

        return properties.some(p => p.id.get() === c.propertyId.get() && p.fieldType.get() === FieldType.SELECT);
      }, validator.properties).valueId.required();
    });

    // validate entire state
    Validation(state).valid();
    
    // validate portion of state (eg: navigation/tabs)
    Validation(state.roles).valid();

    // validation multiple portions (eg: multiple fields on same tab)
    Validation(state).valid(['roles','notifications']);

    // downstream component can check if it is required or valid
    const validation = Validation(state.field);

    const form = <Input error={!validation.valid()) value={state.field.get()} required={validation.required()} />

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2020

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #97   +/-   ##
=======================================
  Coverage   90.83%   90.83%           
=======================================
  Files           1        1           
  Lines         480      480           
  Branches      132      132           
=======================================
  Hits          436      436           
  Misses         44       44           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1585a1...862d582. Read the comment docs.

@parisholley
Copy link
Contributor Author

i made a attempt at extracting the fluent'ness but ran into some typing issues.. though it may be possible with right magic. also realizing as i go that my scope is increasing a bit (eg: marking things as required) which start to feel more like a form plugin that pure validation.

new api addition:

  Validation(state.properties).conditions.when(({ propertyId }, properties) => {
    if (!propertyId) {
      return false;
    }

    const property = properties.find(p => p.id === propertyId);

    return property.fieldType === FieldType.SELECT;
  }).valueId.required();

the when method comes in handy when dealing with form state with cross dependencies. i'm building a "dynamic form ui" which allows a user to optionally show a "property" based on either the existence of another property (eg: checkbox) or based on the value chosen for that property (eg: select box). to keep the UI smooth, i need to conditionally apply validation depending on if the property they are referencing is of a certain type, otherwise, ignore.

Copy link
Owner

@avkonst avkonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to put in code the other idea, which does not require a proxy. We may come up with 2 new validation plugins :) The one old can remain as deprecated. But give me some time for it.

plugins/Validation/src/Validation.ts Outdated Show resolved Hide resolved
@parisholley
Copy link
Contributor Author

in retrospect, i don't remember why i actually needed to downgrade anymore.. any validation callbacks should be listening to the state via get() incase of changes... took it out and still works fine

@avkonst
Copy link
Owner

avkonst commented Aug 26, 2020

"any validation callbacks should be listening to the state via get() incase of changes" - exactly - validation functions should read the state and leave traces of what they touched, so hookstate can detect when and what should be rerendered.

plugins/Validation/src/Validation.ts Outdated Show resolved Hide resolved
plugins/Validation/src/Validation.ts Outdated Show resolved Hide resolved
readonly severity: ValidationSeverity;
interface NestedState {
state: State<any>;
parent?: NestedState;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this relationship may cause large parent object being hold in RAM...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NestedState is transient within the scope of a valid call.. this exists because I ran into issues reusing the State (at root) and the re-render events firing off properly. potentially, this may not be needed the reverse tree walk can be eliminated in conditionalValid, by just doing a lookup each time, but will need to benchmark the performance hit.

}

const PluginID = Symbol('Validate');
type NestedValidator<Root, T> = T extends string ? SingleValidator<T> :
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only string? where are numbers, booleans, undefined and null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why undefined/null?

plugins/Validation/src/Validation.ts Show resolved Hide resolved
plugins/Validation/src/Validation.ts Outdated Show resolved Hide resolved
plugins/Validation/src/Validation.ts Outdated Show resolved Hide resolved
};
}

export function ValidationAttach<T>(state: State<T>, config: ((validator: NestedValidator<T, T>) => void)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is getting close to what I was thinking about. It would be still possible to achieve the same without proxy, but maybe it is the other story.

As we discussed, you need to move your code to the Validation2 plugin, because it breaks compatibility with the previous one.

Also when you are ready. Add the plugin to docs and a code sample like for other plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i'll create a new PR once i'm closer to final state with a new plugin folder.

w/r/t object config vs proxy... take a look at the example validation i have in PR description, in particular the conditional behavior. i feel like trying to replicate that as a POJO would be incredibly verbose.

at least this latest version, the cost of the proxy is only in one place (on attach), so don't imagine there is perf loss going a different route.

@avkonst avkonst closed this Dec 17, 2022
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.

None yet

3 participants