Skip to content

[WIP] Typescript definition for v6 #1318

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

Closed
wants to merge 23 commits into from
Closed

[WIP] Typescript definition for v6 #1318

wants to merge 23 commits into from

Conversation

CarsonF
Copy link

@CarsonF CarsonF commented Jul 8, 2016

Typescript definition for v6 RC 4

This allows users to have the definitions just from installing the repo from npm, which is 👍 👍 💯 .

@erikras I don't mind helping maintain this, if you would just ping me before release I can do the definition updates. Or you can do them yourself and I can look over them :) It should be pretty easy to do small tweaks.

This is very close to complete. I just need to test:

  • FieldArrays
  • Reducer plugin
  • validate's 2nd parameter (may have been a bug that was fixed)

@blakeembrey Would love your thoughts and verification that I'm doing the react dependency right.

EDIT: Updated for RC4

@erikras
Copy link
Member

erikras commented Jul 8, 2016

While personally a TS virgin, I heartily approve of this PR. Let me know when you feel it is complete.

@calvin
Copy link

calvin commented Jul 10, 2016

@CarsonF heads up! I'm getting the following errors using your tsd.

ERROR in /home/user/typings/modules/redux-form/index.d.ts
(834,58): error TS7006: Parameter 'from' implicitly has an 'any' type.

ERROR in /home/user/typings/modules/redux-form/index.d.ts
(834,64): error TS7006: Parameter 'to' implicitly has an 'any' type.

@bbenezech
Copy link
Contributor

bbenezech commented Jul 10, 2016

@CarsonF Using '?' for FormProps definitions is a hack that helps with compilation but it won't work with TS2 strictNullChecks.
For example, TS will complain that this.props.fields can be undefined (since the prop is optional in definition).

We should be using correct HOC definition (merge Props) like Redux' connect:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/react-redux/react-redux.d.ts#L26-L73

It was added with this commit DefinitelyTyped/DefinitelyTyped@206c964 by @use-script

It forwards Redux's props (like state selections or dispatch) in addition to the decorated component's props.

That way you don't need to artificially make HOC's props optionals in order to pass compilation.

Unfortunately I don't feel confident enough in my TS skills to provide any kind of help here.

@use-script would you consider helping us? That would be awesome 🎉

@CarsonF
Copy link
Author

CarsonF commented Jul 11, 2016

@calvin Yeah those are from the arrayMove action creator I just added. @erikras That one isn't documented and I couldn't figure out from source what from and to parameters where supposed to be. Strings?


@bbenezech Ah good point. Ironically I have problems using react-redux's connect function as a decorator. TS has declared that decorators cannot mutate the class definition (more info at TypeScript#4881).

declare type ClassDecorator = <TFunction extends Function>(target: TFunction) => TFunction | void;

I've looked at that block of code several times too and it's hard to wrap my head around. I'll see what I can do to fix this though.
I've gotten around this before with (target: T) => T & DecoratedT. Although that's still a hack too because it doesn't return T only DecoratedT but it satisfies the compiler.

erikras added a commit that referenced this pull request Jul 11, 2016
@erikras
Copy link
Member

erikras commented Jul 11, 2016

That one isn't documented and I couldn't figure out from source what from and to parameters where supposed to be. Strings?

Integers. The indexes in the array.

@bbenezech
Copy link
Contributor

@CarsonF
I had 2 issues with redux decorators:

  1. connect()(MyComponent) doesn't require dispatch; Dispatch<any> prop and doesn't satisfy it. You need to add it to your props with a ?
    I fixed the issue with export function connect<TOwnProps>(): ComponentDecorator<{dispatch: Dispatch<any>}, TOwnProps>
    I then pass TOwnProps like connect<TOwnProps>()(MyComponent)
  2. The other signature overloads won't work without explicitly type casting the connect function (like above) unless TS can infer TOwnProps with mapStateToProps(state, props: TOwnProps).

For redux-form, we should probably require passing ownProps explicitly and keep ReduxFormProps optionals so that we don't need to redeclare them all in user land.
Unless we Inform user there Component's prop type should extend ReduxFormProp.
Which is better for autocomplete.

@rluiten
Copy link
Contributor

rluiten commented Jul 11, 2016

@CarsonF I have pulled in your type definition and adjusted my code to match names it mostly looks good.

I have temporarily hacked my BaseFieldProps to allow me to pass id, type and value at moment as I need to be able to pass in 'value' as a minimum this way at the moment due to what I believe may be a bug.

One larger issue a bug in Field maybe ?

  1. Currently it appears it is not possible to pass in the 'value' property to Field via the props property.

This works fine for defining members of my radio buttons.

<Field name={name} component="input" id={fieldTrueId} type="radio" value="true" />

This does not work at, it breaks because the 'value' field in props is not set on the creatd input element in the DOM. I think this one may be a bug in Field for handling the property.

<Field name={name} component="input" props={{value: 'false', id: fieldFalseId, type: 'radio'}} />

One smaller issue.

  1. I think its nicer to have the 'id', 'type', 'value' on BaseFieldProps so they can be passed to Field rather than forced to use the 'props' property. There are probably a long list of other nice to have attributes for different user cases and different components passed to Field so maybe we need to make BaseFieldProps generic ? (not sure how far reaching that change would be)

I do realise Field does lookups and gets 'value' from the current state of form fields maybe that explains the difference between the direct value="" and the props={{value: ''}}.
For now I have created a RadioInput component to use in the this may be a better choice anyway longer term. Turns out this isnt working somehow using my RadioInput compnent means redux-forms isnt deriving 'checked' value property for me. For now I have resorted to adding value?: string; to ReduxBaseForm.

@CarsonF
Copy link
Author

CarsonF commented Jul 12, 2016

@rluiten I think you are having different problems. You shouldn't need to set the value on the Field. You should use the defaultValue prop for that field or initialValues on the form config.

@rluiten
Copy link
Contributor

rluiten commented Jul 12, 2016

@CarsonF it is a radio button, the value is the value that is set to the identifier name if the radio button is checked or inversely in redux-form. This value is looked up by redux-form and used with type and the current model to derive the checked property for the control. I don't see a way around it, at the moment the only extra field on baseFieldProps I need without modifying redux-form is 'value'.

@rluiten
Copy link
Contributor

rluiten commented Jul 14, 2016

@CarsonF I have created a branch in my fork of redux-form with a very simple change to examples/src/SimpleForm in redux-form repo.

I changed it so that value property is delivered via the props property and the radio button no longer operates correctly. This is why at least at the moment I think we need value on BaseFieldProps

It may be a bug in redux-form Field but I had a dig around but had no luck making progress there.
I am happy to raise it as an issue if you think thats the better avenue ?

Here is reference to the commit that makes Field not work right for radio input buttons.
rluiten@cf11587

I would have liked to run up a codepen, jsfiddle or jsbin or something with a runnable example but it didn't appear feasible at the moment with no readily accessible version of rc3 of redux-form around.

@CarsonF
Copy link
Author

CarsonF commented Jul 14, 2016

@bbenezech I've looked again and I don't think this is possible. Yes what you are proposing is more correct, but if it doesn't compile it doesn't matter. See here: DefinitelyTyped/DefinitelyTyped#9951

I personally would rather have them optional and be able to use the function as a decorator than the other way around. If you want to enable strict null checks, you can using the non-null assertion operator. ie: this.props.pristine!

@CarsonF
Copy link
Author

CarsonF commented Aug 1, 2016

@erikras I'm trying to address @rluiten's situation. How does Field's defaultValue differ from component's value?

@erikras
Copy link
Member

erikras commented Aug 1, 2016

defaultValue is the answer to the question: "What should I use as a value if value is undefined?"

It defaults to '', but if your component wanted a Number, it could be 0, or for a Date, it could be new Date() (today).

So as far as typing is concerned, it can be anything.

@CarsonF
Copy link
Author

CarsonF commented Aug 1, 2016

Should've reread the doc 🤦

Did you intend to have value passed into Field directly? Seems like it is bypassing redux store.. Are radio buttons an edge case maybe?

@CarsonF
Copy link
Author

CarsonF commented Aug 1, 2016

@rluiten You could do this:

import * as React from 'react';
import { Field } from 'redux-form';

export class InputField extends React.Component<React.HTMLAttributes, {}> {
  render() {
    // This is how they say to define react components that have generics
    // ...I don't really like it either.
    // Go upvote https://github.com/Microsoft/TypeScript/issues/6395
    type InputField = new () => Field<React.HTMLAttributes>;
    const InputField = Field as InputField;
    return (
      <InputField component='input' {...this.props} />
    );
  }
}

// Then:
<InputField type="hidden" .../>

EDIT: Changed example to an actual class.

@erikras
Copy link
Member

erikras commented Aug 1, 2016

Did you intend to have value passed into Field directly? Seems like it is bypassing redux store.. Are radio buttons an edge case maybe?

Bingo. It's only used for radio buttons.

@rluiten
Copy link
Contributor

rluiten commented Aug 1, 2016

Thanks for having a look at this case, I really don't want a custom type definition when 6 is released.

It appears that InputField as it stands now does not compile for me and I don't understand the error at the spread attribute to fix it at the moment. I would have expected the casting of Field as the new InputField type was the point of the exercise to address types for typescript.

<InputField component="input" {...this.props} />

The errors is.

error TS2606: Property 'value' of JSX spread attribute is not assignable to target property.

Aside.
I did try out passing in "true" and "false" to the Field using defaultValue attribute as the comments indicated it might pick up the value but that does not work either. The resultant dom value for attribute 'value' is then 'on' for both 'true' and 'false'

@CarsonF
Copy link
Author

CarsonF commented Aug 2, 2016

@rluiten My example compiles with my definition. Do you not have a value property on your Field class?

@CarsonF
Copy link
Author

CarsonF commented Aug 2, 2016

notices rc4 @erikras I think you're just trying to give me more work lol

@erikras
Copy link
Member

erikras commented Aug 2, 2016

Yeah, sorry about that. Better to have a breaking change than a shitty API. Blah, eggs, blah, omelet.

@waynebrantley
Copy link

@CarsonF So you are saying I have to define this new InputField type just so I can use this? Seems pretty messy - I guess that is a typescript thing? Seems like you would have the Field definition in your typings implement the HtmlAttributes interface. (In Material UI, you would know not to use them - but they ARE valid in Redux-Form so I think they should be there)

Also, what about the ReduxForm call at the bottom?

@CarsonF
Copy link
Author

CarsonF commented Aug 16, 2016

@waynebrantley Yes you are right. I was a bit uncomfortable with it to begin with, and hearing the feedback from you and a couple others gave me motivation to work with it some more. I think I have a solution. Commits inbound!

BTW Sorry if I came off short. I appreciate your input! 👍

@erikras
Copy link
Member

erikras commented Aug 17, 2016

Making more work for you, @CarsonF. #1449 #1554 😍

@erikras erikras closed this Aug 25, 2016
@CarsonF
Copy link
Author

CarsonF commented Aug 25, 2016

@erikras What's up?

@huan086
Copy link
Contributor

huan086 commented Aug 25, 2016

To support forms that are stateless component, I had to change the FormDecorator to

Update: the return signature is still wrong with this change

interface FormDecorator<FormData extends DataShape, P, S> {
  <T extends (typeof Component)>(component: T | StatelessComponent<P>): T & Form<FormData, P, S>;
}

To suppress errors such as Generic type \'ReactEventHandler\' requires 1 type argument(s)., I added <any> behind those generic types

@huan086
Copy link
Contributor

huan086 commented Aug 26, 2016

Fields is missing from definition

interface BaseFieldsProps {
  /**
   * An array of string paths, in dot-and-bracket notation, corresponding to a value
   * in the form values. It may be as simple as 'firstName' or as complicated
   * as contact.billing.address[2].phones[1].areaCode.
   *
   * Required but made optional so interface can be used on decorated components.
   */
  names?: string[];

  /**
   * A Component, stateless function, or string corresponding to a default
   * JSX element.
   *
   * Required but made optional so interface can be used on decorated components.
   */
  component?: ComponentConstructor | "input" | "select" | "textarea",

  /**
   * If true, the rendered component will be available with the
   * getRenderedComponent() method. Defaults to false. Cannot be used if your
   * component is a stateless function component.
   */
  withRef?: boolean;

  /**
   * Formats the value from the Redux store to be displayed in the field input.
   * Common use cases are to format Numbers into currencies or Dates into a
   * localized date format.
   */
  format?: Formatter;

  props?: Object;

  /**
   * Parses the value given from the field input component to the type that you want
   * stored in the Redux store. Common use cases are to parse currencies into Numbers into
   * currencies or localized date formats into Dates.
   */
  parse?: Parser;
}

export class Fields<FieldCustomProps> extends Component<BaseFieldsProps & FieldCustomProps, {}> {
  /**
   * true if the current value is different from the initialized value,
   * false otherwise.
   */
  dirty: boolean;

  /**
   * The names prop that you passed in.
   */
  names: string[];

  /**
   * true if the current value is the same as the initialized value,
   * false otherwise.
   */
  pristine: boolean;

  /**
   * The current value of the field.
   */
  values: { [name: string]: FieldValue };

  /**
   * Returns the instance of the rendered component. For this to work, you must
   * provide a withRef prop, and your component must not be a stateless function
   * component.
   */
  getRenderedComponent(): Component<{[name: string]: WrappedFieldProps} & FieldCustomProps, any>;
}

@waynebrantley
Copy link

@erikras @CarsonF
Why did this get closed??

@CarsonF
Copy link
Author

CarsonF commented Aug 31, 2016

I'm not sure. I'm still working on this. Just having a busy few weeks.

@aliatsis
Copy link

any progress here?

@radziksh radziksh mentioned this pull request Sep 19, 2016
@LaurensRietveld
Copy link

@CarsonF: just trying out your defs. Would you like me to post any issues in this thread, or submit issues in your fork. Simple issue: the Selector interface returns an object, where it can also return a single value (see http://redux-form.com/6.0.5/examples/selectingFormValues/)

@CarsonF
Copy link
Author

CarsonF commented Sep 29, 2016

@LaurensRietveld Post them here. I'll try to take a look at that.

I've just been busy lately. Still haven't forgotten about this.

@grovesNL
Copy link

grovesNL commented Oct 2, 2016

I have the same issues as @waynebrantley, we should be able to use the examples directly without wrappers. The props should be determined by an intersection of component props and Field props ideally.

@joewood
Copy link

joewood commented Oct 10, 2016

Any progress with this? Can what's been done so far be merged and raise new tickets for immutable etc...

@bbenezech
Copy link
Contributor

My fork: https://gist.github.com/bbenezech/8ebbced86c0f357d09e4000635eea275
Complete 100% typesafe redux-form example: https://gist.github.com/bbenezech/528c7057c62300d41f51b212f0a92ad2

If someone need a FieldArray example, I can help too, I have some in my codebase.

Let's write and exchange some examples through gist, TS with redux is non-trivial, and RF6 with the inversion of control was specially challenging for me.

The TS definition should be merged, it works well, thanks a lot @CarsonF 🎊

@joewood
Copy link

joewood commented Oct 16, 2016

That's great - @bbenezech could you add the same declarations for "redux-form/react-native" module name too?

@bbenezech
Copy link
Contributor

@joewood Sorry I've never used RN, I don't know what it implies and I cannot test it.

@MichaelBitard
Copy link

@bbenezech nice job, I was using @CarsonF commit for my typings.json github:CarsonF/redux-form/index.d.ts#0bea952 and now I'll try to use yours. I you have examples with dynamic field arrays with initial values I'm really interested!

@bbenezech
Copy link
Contributor

@MichaelBitard the typings are really similar: I just added some API that was missing and that I needed. All credits to @CarsonF

To push new values, I use

this.props.fields.push({ firstName: 'Sherlock', lastName: 'Holmes' })
// To hydrate I don't do anything special
<PatientForm
  handleSave={this.handleSave}
  initialValues={{ users: [{ firstName: 'Sherlock', lastName: 'Holmes' }] }}
  form="users"
/>

I am not sure I understand the issue and how it is related to typings?

@zakdances
Copy link

Doesn't seem like this has been merged yet.

@IRus
Copy link

IRus commented Nov 1, 2016

Sooo, after all we doesn't have definitions?

I checked this repository and DefinitelyTyped.

@mr-pinzhang
Copy link

is this issue completed?

@CarsonF
Copy link
Author

CarsonF commented Dec 17, 2016

We're waiting for DefinitelyTyped people to merge my PR. I don't think Eric wants to maintain the definition here.

@lock
Copy link

lock bot commented Jun 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.