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

Additional generics #408

Merged

Conversation

Austin-Stowe
Copy link
Contributor

@Austin-Stowe Austin-Stowe commented Dec 3, 2022

First discussed in #406.

Purpose

To add generic support for custom Field types used for access when creating custom value editors.

Code Changes

packages/ts/src/props.ts

ValueEditorProps

  • Update interface from ValueEditorProps extends SelectorOrEditorProps to ValueEditorProps<F extends Field = Field> extends SelectorOrEditorProps
  • Pass F type to fields in interface to inherit any custom fields added to the Field type by way of the [x: string]: any available in NameLabelPair
    • This can also be used to add typing to the context field that is available
    • Useful mostly for adding strongly typed additional attribution to fields

@Austin-Stowe
Copy link
Contributor Author

Austin-Stowe commented Dec 3, 2022

The workflows are failing, and that's to be expected. I was also experimenting with changing the operators type to

export interface ValueEditorProps<F extends Field = Field> extends SelectorOrEditorProps {
  field: F['name'];
  operator: NonNullable<F['operators']>[number] extends NameLabelPair 
    ? NonNullable<F['operators']>[number]['name']
    : NonNullable<F['operators']>[number] extends OptionGroup
    ? NonNullable<F['operators']>[number]['options'][number]['name']
    : string;
  value?: any;
  valueSource: ValueSource;
  fieldData: F;
  type?: ValueEditorType;
  inputType?: string | null;
  values?: any[];
  listsAsArrays?: boolean;
}

So that if the props aren't using a custom type the operator should default to string but it is causing some testing to fail as expected. Realistically, for the use case I mentioned, only the fieldData field of the interface would benefit from passing in a custom Field type with the way I intend to use it. The other fields being updated (name, operator, etc.) are just for passing that along as well since it can't hurt to pass along more context in the case someone else like me decides to customize the default types.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 3, 2022

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 b5f8fa6:

Sandbox Source
react-querybuilder-ci Configuration
react-querybuilder-dnd-example Configuration
react-querybuilder-antd-example Configuration
react-querybuilder-bootstrap-example Configuration
react-querybuilder-bulma-example Configuration
react-querybuilder-chakra-example Configuration
react-querybuilder-material-example Configuration

@jakeboone02 jakeboone02 changed the title 406 generic field usage Additional generics Dec 3, 2022
@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #408 (b5f8fa6) into main (f68d6e8) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #408   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         3089      3087    -2     
  Branches      1480      1480           
=========================================
- Hits          3089      3087    -2     
Impacted Files Coverage Δ
...ackages/react-querybuilder/src/utils/queryTools.ts 100.00% <0.00%> (ø)
...ges/react-querybuilder/src/utils/transformQuery.ts 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jakeboone02
Copy link
Member

I think the workflows weren't actually failing (and they shouldn't since you've only modified types and not execution), they just weren't running because it's a draft PR. Anyway don't worry about those for now.

I don't think we need to be so sophisticated with the type for the operator property, especially since, as I alluded to in #406 (comment), inferring the type from a field's operators property is insufficient. That should probably be another generic that defaults to string. I was thinking we'd end up with something simpler like this:

export interface ValueEditorProps<
  F extends Field = Field,
  O extends string = string,
  V = any,
  Vals = any> extends SelectorOrEditorProps {
  field: F['name'];
  operator: O;
  value?: V;
  valueSource: ValueSource;
  fieldData: F;
  type?: ValueEditorType;
  inputType?: string | null;
  values?: Vals[];
  listsAsArrays?: boolean;
}

Inference from the operators could be done in userland if necessary, like this maybe (I haven't really tested it, but here's a TS playground):

const operators = [{ name: '=', label: '=' }, { name: '!=', label: '!=' }] as const;

type Op = (typeof operators)[number]['name'];

const CustomValueEditor = (props: ValueEditorProps<Field, Op>) => {
  /* ... */
};

@Austin-Stowe
Copy link
Contributor Author

Austin-Stowe commented Dec 3, 2022

I actually like that a lot better, it's easier to understand than my mess and is more aligned with the generics in the RuleType which I'm also leveraging. Like you've said, it also gives the user the ability to define the types like I do, which I'll honestly end up doing as well if this is the direction it goes.

I'll update this PR with changes similar to that if not those exactly in the next few days

@jakeboone02
Copy link
Member

Here's an idea for generics on the Field interface. We might want to do the same for the values property as I've done here for operators, but it's probably not important.

-export interface NameLabelPair {
-  name: string;
+export interface NameLabelPair<N extends string = string> {
+  name: N;
   label: string;
   [x: string]: any;
 }

 export type OptionGroup<Opt extends NameLabelPair = NameLabelPair> = {
   label: string;
   options: Opt[];
 };

-export interface Field extends NameLabelPair {
+export interface Field<
+  N extends string = string,
+  OpName extends string = string,
+  OpObject extends NameLabelPair = NameLabelPair<OpName>
+> extends NameLabelPair<N> {
   id?: string;
-  operators?: NameLabelPair[] | OptionGroup[];
+  operators?: OpObject[] | OptionGroup<OpObject>[];
   valueEditorType?: ValueEditorType | ((operator: string) => ValueEditorType);
   valueSources?: ValueSources | ((operator: string) => ValueSources);
   inputType?: string | null;

@Austin-Stowe
Copy link
Contributor Author

That will certainly work better than my current existing flow for the way I use the Field type, and will be pretty simple to build checks against for custom components.

I should be able to make these changes tonight and commit them, apologies for the delay in responding .

@jakeboone02
Copy link
Member

jakeboone02 commented Dec 8, 2022

This looks good so far. The one thing I'm wondering about is the order of generics on Field since you can't "skip" defining a generic. (For example, if you only want to narrow the first and third generics of an interface, you have to explicitly assign the second generic even it has a default.)

I'm thinking it might be better to have them in this order, with ValueName coming before OperatorObj:

 export interface Field<
   FieldName extends string = string,
   OperatorName extends string = string,
+  ValueName extends string = string,
   OperatorObj extends NameLabelPair = NameLabelPair<OperatorName>,
-  ValueName extends string = string,
   ValueObj extends NameLabelPair = NameLabelPair<ValueName>
   > extends NameLabelPair<FieldName> {
   // ...
 }

This way the user could define the types for the name properties of the field itself, the operators, and the values, without having to bother with the object types (which I think won't be narrowed/extended as often).

Implementation would be like this:

type MyFieldNames = 'f1' | 'f2';
type MyOperators = '=' | '!=';
type MyValues = 'v1' | 'v2';

type MyField = Field<MyFieldNames, MyOperators, MyValues>;

The way it is now would require the user to do this:

type MyField = Field<MyFieldNames, MyOperators, NameLabelPair<MyOperators>, MyValues>;
//                                              ^ unnecessary; equivalent to default

@Austin-Stowe
Copy link
Contributor Author

That's a good catch, the positional generic arguments make more sense in that pattern. Pushing that update now

@Austin-Stowe Austin-Stowe marked this pull request as ready for review December 9, 2022 03:01
@jakeboone02 jakeboone02 merged commit 5e80125 into react-querybuilder:main Dec 9, 2022
@jakeboone02
Copy link
Member

@all-contributors please add @Austin-Stowe for code.

@allcontributors
Copy link
Contributor

@jakeboone02

I've put up a pull request to add @Austin-Stowe! 🎉

jakeboone02 added a commit that referenced this pull request Dec 9, 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

2 participants