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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Minor fix on ValidationRule to match AsyncValidator #21250

Merged
merged 2 commits into from Feb 6, 2020

Conversation

hansololai
Copy link

@hansololai hansololai commented Feb 5, 2020

馃 This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

馃敆 Related issue link

close #21143

馃挕 Background and solution

In a scenario where one could store the ValidationRule and use in both in front end with Antd Form, and also could be used in the back end with async-validator alone. The rules could not be shared because the newest version async-validator v3's type definitionRuleItemlink here does not match the ValidationRule defined in Antd v3.

After some investigation I found out rc-form still depends on async-validator, which did not have the Typescript definitions (async validator added Typescript support later). So Antd could not directly reexport the RuleItem, but have to manually define ValidationRule. But it is not the same.

Without introducing new features, I just corrected some fields on ValidationRule to be more consistent with the RuleItem. Even though the RuleItem is for async-validatorv3, it was not changed very much after it was introduced, indicating the RuleItem still works for v1.11.

To have RuleItem and ValidationRule to be fully assignable, the RuleItem may also need to be changed.

import { ValidationRule } from 'antd/lib/form';
import { RuleItem, RuleType } from 'async-validator';
import { ReactNode } from 'react';


// ValidationRule.type is string, which is not assignable to RuleType
// Validation.enum is string|string[], not assignable to RuleItem
// These two are included in this PR
interface VR  extends Omit<ValidationRule,'type'|'enum'>{
  type: RuleType;
  enum?: (string|number|boolean)[];
}
// ValidationRule.message is ReactNode, RuleItem.message is string
// ValidationRule.pattern is RegExp, RuleItem.pattern is string. (actually async-validator handles pattern and string. 
// To fix these, async-validator needs a PR. 
interface RI extends Omit<RuleItem, 'pattern'|'message'>{
  pattern?: RegExp ;
  message?: ReactNode;
}

const a = {} as VR;
const b: RI = a;

馃摑 Changelog

Changed the ValidationRule type and enum field.

  1. The type is not a string enum. This should not affect you if you did not use types that is not handlable by async-validator.
  2. BREAKING CHANGE The enum field now only support array of (string, number, boolean), it no longer support simple string. Which did not make sense anyways.
Language Changelog
馃嚭馃嚫 English 鈽戯笍
馃嚚馃嚦 Chinese

鈽戯笍 Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@hansololai hansololai changed the title Minor fix on ValidationRule to match AsyncValidator fix: Minor fix on ValidationRule to match AsyncValidator Feb 5, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d0dbc41:

Sandbox Source
antd reproduction template Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 161275d:

Sandbox Source
antd reproduction template Configuration

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #21250 into 3.x-stable will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           3.x-stable   #21250   +/-   ##
===========================================
  Coverage       97.82%   97.82%           
===========================================
  Files             286      286           
  Lines            7754     7754           
  Branches         2185     2188    +3     
===========================================
  Hits             7585     7585           
  Misses            169      169
Impacted Files Coverage 螖
components/form/Form.tsx 91.3% <酶> (酶) 猬嗭笍

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 d3102bb...161275d. Read the comment docs.

@afc163 afc163 merged commit f7e7ce9 into ant-design:3.x-stable Feb 6, 2020
@hansololai hansololai deleted the fix_validator branch February 6, 2020 03:30
@Kamahl19
Copy link
Contributor

@afc163 @hansololai This change is actually not backward compatible. After upgrading to 3.26.9 my app wont build.

Type '({ required: boolean; message: string; } | { type: string; message: string; })[]' is not assignable to type 'ValidationRule[]'.
  Type '{ required: boolean; message: string; } | { type: string; message: string; }' is not assignable to type 'ValidationRule'.
    Type '{ type: string; message: string; }' is not assignable to type 'ValidationRule'.
      Types of property 'type' are incompatible.
        Type 'string' is not assignable to type '"string" | "number" | "boolean" | "object" | "url" | "email" | "date" | "method" | "regexp" | "integer" | "float" | "array" | "enum" | "hex" | "any" | undefined'.  TS2322

@hansololai
Copy link
Author

@Kamahl19 You are right. The type has been restricted to not just any string, but have to be one of the options in the union type. I did it to match async-validator. What do you think is the best way to handle this? @afc163 . If we want to only do any breaking changes on major release, like 4.0, then we can revert this PR. because 4.0 already handled this issue.

I feel like it is so hard to update any Typescript definitions because any update on Types can be backward incompatible.

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