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

migrate to redux form v6 #18

Closed
justingreenberg opened this issue Jun 18, 2016 · 34 comments
Closed

migrate to redux form v6 #18

justingreenberg opened this issue Jun 18, 2016 · 34 comments
Assignees
Projects
Milestone

Comments

@justingreenberg
Copy link

justingreenberg commented Jun 18, 2016

first of all, thank you for your awesome work on this project! 💯

since redux form v6 release is right around the corner, it probably makes sense to consider the impact of these api changes sooner than later... https://github.com/erikras/redux-form/releases/tag/v6.0.0-alpha.15

http://redux-form.com/6.0.0-alpha.15/docs/MigrationGuide.md:

In v5, only the outer form component was connected to the Redux state, and the props for each field were passed in via the form component. The problem with this is that the entire form component had to rerender on every single keypress that changed a form value. This was fine for small login forms, but lead to extremely slow performance on larger forms with dozens or hundreds of fields.

In v6, every single field is connected to the Redux store. The outer form component is also connected, but is connected in such a manner that does not require it to refresh every time a value changes.

Because of this inversion of control, there is no incremental upgrade path. I would love to provide new API and provide deprecation warnings on the old API, but there is just no path from here to there that allows for such a transition.

without digging too deep into both APIs, i wanted to ask if you had considered these changes and plan on supporting the new api/optimizations?

@andrerpena
Copy link
Member

andrerpena commented Jun 18, 2016

Thanks for the kind words and for the heads up.

I admit I wasn't even aware there was a v6 going on and I agree that migrating sooner is better than later. Yes, I want to migrate to v6 and I'll have this as a priority.

I'll start taking a look at it tomorrow and I'll post updates here as I go forward.

Thanks again.

@andrerpena
Copy link
Member

@JonatanSalas , this something we need to discuss ASAP. @erikras is reasling the v6 shortly.

@andrerpena
Copy link
Member

Just so you know @erikras and @justingreenberg , @JonatanSalas is doing an excellent job on this repo and implementing MaterialUI support. It's gonna rock!

@andrerpena
Copy link
Member

andrerpena commented Jul 19, 2016

@justingreenberg , thanks for the suggestion. 3 points:

  • Autoform is a well established name that I believe people look for at the internet. meteor-autoform is quite popular.
  • We are based in Redux-Form and it's well credited in the description and in the docs.
  • I didn't really like your alternatives, sorry :)

Thank you anyway.

@justingreenberg
Copy link
Author

justingreenberg commented Jul 19, 2016

all good, nothing to do with you guys not giving proper credit or the findability of lib, but it's definitely an open-source convention to include the name of base library in ecosystem extensions

op: #21 (comment)

redux-form-factory has a nice ring to it, in my humble opinion 😄

@JonatanSalas JonatanSalas added this to the v1.0.0 milestone Jul 20, 2016
@JonatanSalas
Copy link
Member

@andrerpena I have been taking a look at it, and a lot of things changed. Now everything turns around a component called Field that connects to the store and manage the fields and render a component you pass in.

This changes will make that our codebase for the UIs change a lot, after thinking I have been making an abstraction so we can convert all UI components to work with version 6.

We can create a high order component that wraps a UI component into a Field component so we don't have to make a big refactor in all the components.

@andrerpena
Copy link
Member

@JonatanSalas , I'm glad you took time to look into it. A HOC may be a solution.
Can you please wait until this weekend before you start working on it? I'd like to create a POC with v6 and then discuss with you just to make sure we're heading in the right direction. This decision is critical.
Thank you.

@JonatanSalas
Copy link
Member

@andrerpena It's a proposal only, we can do this together if you want! 😄

@andrerpena
Copy link
Member

Ok. I'm installing RF 6 now and I'll play around with it and I'll come back with a plan today

@andrerpena
Copy link
Member

@JonatanSalas and @danigomez Ok.. This doesn't seem to be the nightmare I was previously expecting.. I'm creating a fork in which I'm making the simple provider in the demo to work. After that I'll move on the Bootstrap.

@JonatanSalas
Copy link
Member

JonatanSalas commented Aug 27, 2016

@andrerpena the big difference is the usage of a component called Field to sync with the store (Obviously that there are a lot of changes, but I think the big one is this).. that's why I tell you to add a HOC to wrap this new functionallity and apply to all the components we got on bootstrap-ui

@andrerpena
Copy link
Member

andrerpena commented Aug 27, 2016

@JonatanSalas I'm not sure if that's what you call "adding a HOC to wrap this new functionallity", but what I'm thinking to do is to make the getFieldComponent function from the ComponentFactory to always return a RF Field component wrapping whatever the ComponentFactory returns.

So, if your ComponentFactory returns componentX, the getFieldComponent actually returns

<Field component={componentX} {...otherProps}/>

This is what I'm seeing as the solution now. We just have to make that demo to work :)

@andrerpena andrerpena self-assigned this Aug 27, 2016
@andrerpena
Copy link
Member

Now that the demo is working, I'll start to work on this and I'll keep you informed as I have progress.

@JonatanSalas
Copy link
Member

What you are going to do is a high order component haha

@andrerpena
Copy link
Member

@JonatanSalas I understand HOCs as being components that wrap everything, like the Provider from Redux, or Route from Redux-Router, but if we stretch the concept to accommodate wrapping individual components, than I'm building a HOC 😄

@JonatanSalas
Copy link
Member

@andrerpena Not necessary a HOC wraps everything. Provider and Route aren't HOCs.
Connect from redux and withRouter from react-router are HOCs.

A HOC is used to extend the functionallity of a component, you use a HOC if you have a same logic applied to all components, so in order to not repeat the code you create a function that wraps this functionallity onto the components that share the same logic.

@JonatanSalas
Copy link
Member

@andrerpena the HOC is the es6 replacement for React mixins

@andrerpena
Copy link
Member

Thanks for the explanation. I really didn't know that. Can you please provide an URL where I can look at it?

@JonatanSalas
Copy link
Member

Sure thing! Here you got a good one from Dan Abramov: https://facebook.github.io/react/blog/2016/07/13/mixins-considered-harmful.html#higher-order-components-explained

@JonatanSalas
Copy link
Member

JonatanSalas commented Aug 27, 2016

So we could have a function like this:

import React, { Component, PropTypes } from 'react';
import { Field } from 'redux-form'; 

function withFields(WrappedComponent) {
    return class FieldComponent extends Component {
       render() {
          return <Field component={WrappedComponent} {...WrappedComponent.props}/>;
       }             
    }
}

and use like this:

import React, { Component, PropTypes } from 'react';
import withFields from '../withFields';

class TextBox extends Component {
    {...}
}

export default withFields(TextBox);

@andrerpena
Copy link
Member

andrerpena commented Aug 28, 2016

Thanks @JonatanSalas. Now I understand. And now I agree with you on

Provider and Route aren't HOCs. Connect from redux and withRouter from react-router are HOCs.

Yes, now I know we should return a HOC, but the solution is the same as before: buildFieldComponent from the component factory should always return a Field. The drawback is that the component factory project now will have a reference to Redux-Form but, on the flip side, there's no way around it. I could delegate the convert to Field responsibility to each factory individually but then they would be doing the exact same thing every time. I could also pass the convert to Field to RAF itself, I mean, when it returns a factory, it actually pass a HOC function to it, so that buildFieldComponent will use the HOC function.

TL;DR

I'll start by simply returning a Field from inside buildFieldComponent and solve every problem there's gonna be. After that we can refine the solution.

@andrerpena
Copy link
Member

andrerpena commented Aug 28, 2016

The biggest difference is that AutoFormInternal used to receive a fields array with all RF metadata for each field (name, onChange, onBlue...) and now it doesn't anymore. This is exactly what the Field component is for.

I'm working on it.

andrerpena added a commit that referenced this issue Sep 9, 2016
andrerpena added a commit that referenced this issue Sep 9, 2016
@andrerpena
Copy link
Member

I'll update the progress here as I go further

Progress

  • Make it render and update state based on input, with no errors
  • Validation
  • Metadata processing based on functions
  • Arrays

@JonatanSalas
Copy link
Member

@andrerpena good! Really good! 😄

@andrerpena
Copy link
Member

Just a quick update.

The reason why they refactored the entire thing is that they want to prevent the form from rerendering on every key-press. That makes all sense. The problem is that RAF depends on it because every metadata should be reprocessed every time. That's how the reactiveness of RAF works.

So, in order to circumvent this problem temporarily, I'll use Redux connect to grab the fields from the store and force a rerender on every keypress, just like in RF5.

In the future, we should have a prop on RAF that receives the array of fields that should cause a rerender. If you don't pass this array, then I'll make it behave exactly like RF5. If you pass this array, I'll optimize the connect to only cause a rerender if any of the fields you specify were altered.

I'm travelling to Rio de Janeiro tomorrow for the weekend, so I'll be away until next week. But Monday or Tuesday I'll resume this RF6 migration.

@andrerpena
Copy link
Member

I'm in need for help so I posted this issue on RF6, I hope they answer it, even though I find this unlikely since they got 362 issues 😢

redux-form/redux-form#1904

@JonatanSalas
Copy link
Member

@andrerpena okey, let's wait for a response.

We now can focus on improving our view components library if you want! And adding another features to autoform like i18n, readOnly fields, more validators, support for async validation

@andrerpena
Copy link
Member

ok @JonatanSalas , let's do it

@JonatanSalas
Copy link
Member

Great! Thank you! :)

@Aman725
Copy link

Aman725 commented Jul 12, 2017

Hey @andrerpena @JonatanSalas,
Firstly thank you for your awesome work on this project, it has helped me produce lots of redux forms at a faster pace.
Just wanted to know are there any plans for resuming this RF6/RF7 migration.

@JonatanSalas
Copy link
Member

@Aman725 I was working on a newer version of redux-autoform in a new repository, see https://github.com/redux-autoform/redux-autoform-next.

The trouble is that I've been with a lack of time, and I can't work over it. I'm trying to find some free time to spend and see if it's possible to make a newer release.

If you want to contribute, please, let me know, and I will put in contact with you to see how to achieve a newer release of this library.

@Aman725
Copy link

Aman725 commented Jul 12, 2017

yeah sure count me in will try to spend an hour a day for this project.

@JonatanSalas
Copy link
Member

@Aman725 great! I will invite you to access the next repository.

@JonatanSalas
Copy link
Member

This is currently solved. I'm working on a private repo that will merge with this one when I have more UI-libraries supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
next
TODO
Development

No branches or pull requests

4 participants