v6 proposal regarding loss of focus when stateless functions are passed to Field and Fields

[naw]

Regarding these docs:

You must define the stateless function outside of your render() method, or else it will be recreated on every render and will force the Field to rerender because its component prop will be different. If you are defining your stateless function inside of render(), it will not only be slower, but your input will lose focus whenever the entire form component rerenders.

I understand both the performance and the loss of focus issues that arise when you do something like this:

<Field name="address" component={(props) => (
  <input type="text" {...props.input} />
</Field>

Nevertheless, this is a nice syntax to have. At times, defining the stateless function outside of your render function is fairly inconvenient and undesirable because it moves your code out of the immediate cognitive context. It would be nice to have this syntax, without the performance and loss of focus issues.

The most onerous of the issues is the loss of focus issue, which ultimately is caused because react thinks the functional component is a completely different component each time, which causes react to unmount the old one and mount a new one, rather than just changing the props on the existing one.

The crux of the mounting (and loss of focus) issue is that redux-form is treating these functions as functional components (i.e. passing them to react.createElement) rather than just arbitrary functions that return elements (i.e. JSX).

My proposal is that we introduce a lower-level API to <Field> and <Fields> that allows you to pass a function instead of a functional component:

<Field name="address" render={(props) => (
  <input type="text" {...props.input} />
</Field>

Note this is identical to the former code, except instead of passing our function as the component prop, we're passing it as the render prop. Then, redux-form internally would simply call this function rather than passing it to react.createElement.

This approach allows what I believe was the originally desired syntax for stateless functions, but does not cause the loss of focus issues. Thoughts?

[naw]

This ties into the older discussion here #961, in particular #961 (comment) where @erikras wrote:

It annoys me that the code elegance of <Field name="myField" howToRenderItHereInline={...}/> might be a dead end. It would be so nice if it worked.

[clayne11]

What you're proposing doesn't fix the issue. Every time that Field is rendered you're creating a new function and thus React is not able to reconcile the render function as the same function because it isn't the same function.

There's no way to make this work within React. The way reconciliation works within React makes this impossible.

[naw]

@clayne11 I believe you've misunderstood the crux of how my solution works. I'm suggesting not passing the function to react.createElement, which means what react is reconciling is what is returned by the function, which is other components which indeed are the same.

[clayne11]

Oh alright. I see what you mean. So within the Field render function instead of using React.createElement you would simply call render.

const Field = ({component, render}) => {
  if (render) {
    return render()
  }

  return React.createElement(component)
}

That's the gist of it?

[naw]

Yes, that's the gist of it.

[naw]

FWIW, it's feasible to build this as a wrapper around <Field> in user land, but it's less overhead to implement it directly inside <Field>.

If it's a pattern other people feel is useful, it would be nice to revive this syntax within vanilla redux-form rather than an external wrapper.

Going further, I occasionally even do the same thing with the whole form with a <Form> wrapper like this:

<Form form="myFormName" render={(formProps) => (
  // all your form stuff goes here
)} />

This allows a syntax where you have access to various form-level or field-level props in closures instead of thinking about how to pass them down directly through a chain of component props. Arguably not for all people, and not for all times, but I've found it's a nice syntactical tool to reach for when it's convenient or helpful.

[erikras]

This is how v6 originally treated the function syntax, but in #871, @gaearon convinced me that createElement() was the way to go.

Presumably for this "just call the function" syntax to work, Field would need to ignore changes to the render prop in shouldComponentUpdate(), right? That means that you're going to lose any changes to closure variables in your function, which will lead to very confusing bugs.

[naw]

Yes excellent point about the closures, and for that reason, you definitely couldn't ignore changes to the render prop in shouldComponentUpdate.

That does mean every field that is constructed using the render prop would be re-rendered, so I suppose this is another case of performance vs. convenience. Arguably, the DOM manipulations are the slowest, so re-rendering identical virtual DOM is hypothetically very fast, although I suppose large forms could suffer, so I suppose this is a trade-off that people would have to weigh.

I'm sure you know this, but just to be very clear for anyone following the discussion and also to address the subtleties of #871, functions can be treated in two different ways:

1. functional components. If you're passing the function as a Component in the component prop, then it definitely it needs to be treated as a component and passed to createElement as @gaearon suggested in [v6] PropTypes not checked in custom elements ? #871, and things like propTypes will work as expected.
2. Regular old JavaScript functions that return a value. If you're passing the function as a function in the render prop, then you don't want to pass it through react.createElement. Instead, you want to call it directly. This is fairly analogous to having helper methods in your components that return a portion of your render() result:

class MyComponent extends Component {
  someChunk() {
    return <p>Some Chunk</p>
  }
  render() {
    return (
      <div>
        <p>Main content</p>
        { this.someChunk() }
      </div>
    )
  }

Here there is certainly no expectation that someChunk is to be treated as a component or that defining propTypes on it would work.

I can see that someone could accidentally pass a functional component to the render prop by mistake (instead of the component prop) and have unexpected behavior.

The reality is there are intrinsic subtleties to inline functions:

1. closure values
2. unmounting/remounting if you're using createElement

These subtleties are exactly why we have that big warning in the docs about not using inline stateless functional components.

I don't think there is any way to escape these subtleties, so I suppose what I'm suggesting is rather than warning people not to use inline functions, and rather than avoiding inline functions altogether, warn them that inline functions need to be treated as plain functions (not functional components) and passed to render instead of component.

You make a good point that performance would be affected. My proposal is solving the "loss of focus" issue, not the performance issue.

[naw]

Part of the trouble here is that react is treating a function as a component in a certain cases (i.e. when you pass it to createElement), which makes distinguishing regular functions from functional components awfully subtle. Perhaps it would have been better for functional components to need to be explicitly created as in:

createFunctionalComponent((props) => <p>Render this</p>);

Too late now.

[clayne11]

In light of the discussion in #871 it makes sense to me to leave things as they are. Perhaps it would make more sense to have the component as a child to the Field, which I saw mentioned in another issue:

<Field>
  <MyInput/>
</Field>

This is more idiomatic React and it would help with the issue of people passing in new functions in each render as it's a more common pattern.

[davidkpiano]

Perhaps it would make more sense to have the component as a child to the Field, which I saw mentioned in another issue

That's how it works in React-Redux-Form, if you want to look there for implementation details. (more than willing to help!) It's a pattern that seems to work pretty well.

Regarding rendering custom components, V1 is giving the option to have children-as-a-function:

<Field>
{(fieldValue) => <MyInput ... />}
</Field>

that provides the field. I know performance is a concern, so there's also a new prop: <Field dynamic={false}> that can be set to say "the children are pure and will never change" (e.g., no closured variables, no dynamic children, etc.) and this speeds up performance by letting shouldComponentUpdate only shallowEqual compare for changed props without checking children.

What do you think of those patterns? Would be glad to help!

[erikras]

This is more idiomatic React

I'm not so sure about that statement, but I have looked at how react-redux-form does it (as you suggest), and there is just something about cloning nodes that reeks, perhaps illogically, to me of performance loss. The word "clone" turns me off.

Of course @davidkpiano would post as I was writing this. :-P (I had already linked to his library before his post showed up)

[davidkpiano]

@erikras Yeah, cloning is weird but it's working pretty well 😄 I don't prefer it either but I can't really think of a better way that allows for a succinct API.

[erikras]

V1 is giving the option to have children-as-a-function

This feels (perhaps it is not, but I'm sharing my many-years-of-xml gut reaction) pretty disgusting. I'm sure you recall back when you had to do <Provider>{() => <MyApp/>}</Provider> due to React issues (with context?) with react-redux.

It's unclear to me that passing children-as-a-function is less disgusting than prop-as-a-function.

[davidkpiano]

Got my inspiration from here: https://medium.com/merrickchristensen/function-as-child-components-5f3920a9ace9#.jw39d4gb6

[clayne11]

I'm not so sure about that statement, but I have looked at how react-redux-form does it (as you suggest), and there is just something about cloning nodes that reeks, perhaps illogically, to me of performance loss. The word "clone" turns me off.

I agree with you on that point. I'm not sure if there is actually a performance issue but every time I clone a node I have that same sinking feeling in the pit of my stomach that I'm doing something wrong.

It would be good to get some input from someone more in the know about React performance.

[davidkpiano]

@clayne11 @erikras To be fair, React.cloneElement basically does almost the exact same thing you'd have to do manually in Redux-Form for custom components anyway - copying props over to build a new control.

Also, in V1, the <Control> component will be introduced which uses createElement for standard controls (e.g., input, textarea, select, etc.) instead of cloneElement.

[naw]

Perhaps this is obvious, but "inline function as a child" vs. "inline function as a prop" is purely cosmetic.

The child in JSX is just syntactic sugar for the 3rd argument to createElement, so whether you put the function in the 3rd argument, or you put it in the 2nd argument, you have exactly the same underlying closure issues and the same underlying performance issues, don't you?

[clayne11]

You do. I don't think allowing inline function definition is a good pattern at all and I don't think we should allow / encourage it.

[davidkpiano]

I don't think we should allow / encourage it.

Why? In the most common cases (that is, without closures) pure and idempotent functions as children can be optimized, by having shouldComponentUpdate skip over them.

I'd like to see Redux-Form's API at a point where there isn't a sizable amount of nuances the developer has to "just know" by reading the docs, such as being forced to define the render function outside the <Field> component.

[naw]

@clayne11

Interesting. How awkward would HTML be if you could only nest things 2 levels deep, and you always had to define the child before the parent like this:

MyParagraph:

<p>
  Hello World
</p>

MyBody:

<div>
  <MyParagraph>
</div>

Final Page:

<body>
  <MyBody>
</body>

That's how using redux-form without an inline API feels to me, and I'd gladly trade some performance for a less clumsy API. Just trying to help illustrate my opinion, definitely OK if you disagree.

[naw]

I'm not necessarily opposed to using children instead of a function, but you still have the exact same performance and closure issues because the children themselves are inline, and have access to all of the variables in scope. They are rendered every time (just like the function would be rendered every time), so there is no performance benefit.

<Field>
  <MyInput/>
    <span someProp={somethingFromScope}>Hello</span>
  </MyInput
</Field>

So, children vs. functions is just a cosmetic issue, not an underlying shift in performance or closure issues. To the degree that you can optimize things for one approach, you can optimize them for the other approaches.

The fundamental question here is whether specifying field code inline is a desirable API to have.

[clayne11]

IMO it's not really like that. I'm essentially saying that for each different type of component you want to support, you have to create a re-usable component.

Presumably you want your forms to be consistent throughout your app, so you need to create one of each:

• text input
• date picker
• checkbox
• radio button
• select field
• switch

and maybe a couple others. I don't think it's particularly onerous to ask you to create these re-usable components before passing them into redux-form. You're probably going to be wrapping them with HOCs that will show errors, labels, and perhaps even use a schema to restrict values / pre-populate select fields.

In React in general you're not really supposed to create new functional components inline. Also, redux-form is solving a particular use-case which is high-performance forms that manage validation and state. In order to get these benefits you're trading off some simplicity and boilerplate.

In general with redux you're trading performance / ability to reason about a large app for boilerplate. You have to write a lot of boilerplate with redux, which is no different that what's happening here.

Personally, I'll write some extra boilerplate to significantly decrease the number of bugs that I have in my app and to ensure that I can track down performance problems. If you don't like writing boilerplate then you probably shouldn't be looking at redux-anything as a solution for your app.

That's my two cents at least.

[naw]

Yes I can see (although I don't completely share) your perspective regarding a fairly limited number of reusable form controls. Personally, probably 90% of mine are pre-defined, whereas 10% might be one-off things where inline would be nice.

How about <Fields>, though?

<Fields names={['field1', 'field2']} render={({ field1, field2 }) => (
   some condition based on field1 and field2 ? <OneThing /> : <AnotherThing />
)} />

Would you move every chunk of logic like that into a pre-defined named component/function?

[naw]

I think we can agree reducing bugs and tracking down problems are good goals, so really the difference here seems to be what aspects of coding we feel helps us do that, and whether the library itself should encourage or discourage a given approach.

My proposal supports both approaches, so I suppose the ultimate question is whether supporting both approaches is overall good or bad for the library as a whole, and of course that's not my call to make.

My desired API is feasible through user land wrappers; my proposal was based on the assumption that an inline API was generally desirable to the rest of the community too.

[erikras]

Excellent and thoughtful discussion here, guys. 👍 If only all internet discussions were as well argued and respectful. 😄

Let's define the problem that this issue is trying to address.

---

Problem

It's annoying having to define component render functions outside of render().

---

@naw illustrated this brilliantly with his HTML analogy. Having functions inline always makes for more readable code. However, there are many performance issues, and potentially confusing closure bugs, with this.

I think we have concluded the all the options suck in their own unique way, so we have to decide what tradeoff we want to make.

@naw has discovered that just calling the function rather than using createElement() to create a functional component solves the focus issue. However, this does result in the propTypes problem from #871. How about this:

---

Proposal

redux-form could just look at the function to see if there is a propTypes property, and if there is, it uses createElement(), otherwise it calls the function.

---

What this gets us:

• No need to introduce a new render prop; we can use component.
• It allows a user to write fat arrow components inline in their render() method if they absolutely prefer to trade the performance hit of constant rerendering for "cognitive convenience".
• No closure issues because we're recreating the function on every render.
• Solves the focus issue.
• If you've got propTypes defined, you're already not defining the function inline anyway.
• It follows @gaearon's advice: "You’re generally not supposed to create components inside render()", as it's just a function for how to render, not a component.

The only con I can think of is that it's a bit of a misnomer to have a "not a component, just a how to render" function be passed to a prop called component, but I think it's superior to the introduction of another prop.

What do you think?

[ruiaraujo]

@erikras I like your proposal, as long as 6.0.0 final is cut before. 😄

I would say that stabilising redux-form as it stands should be the priority.

[erikras]

@ruiaraujo That's the thing, though. My proposal would not be a breaking change. If anything, it would repair problems from people misusing the current way.

[ruiaraujo]

@erikras I know it is not breaking hence it can be worked on after the final cut. 😉

[clayne11]

@erikras I don't like that solution. A lot of people are moving to use flow in place of propTypes, and propTypes are also not a requirement for a functional component. Personally I like the way things are now and I don't like this change.

I really think it should just stay the way it is now. Defining anything inline, be it a component or a function, is an anti-pattern in React and should not be encouraged.

[naw]

I agree it would be sticky to use propTypes as a heuristic for whether a function is meant to be used as a functional component or whether it's meant to be used as a function. At it's simplest, a function meant to be used as a functional component is indistinguishable from a normal function, and it's difficult to predict how changes in react in the future might cause subtle differences in behavior if a simple functional component were accidentally treated as a plain function.

Having two props forces users to think about what they're really trying to do; unfortunately, people will still be able to shoot themselves in the foot (which they already doing now, because not everyone reads the warning in the docs).

I think two props is fine (although render might not be the best name for the second prop).

I don't think it's an anti-pattern to pass values to anonymous functions --- it's a powerful technique programmers use all the time, and merely being in react (or redux) doesn't fundamentally change that.

The only reason closures start causing bugs is when you try to cache the function (i.e. ignore it in shouldComponentUpdate) --- it's not intrinsically fragile.

It's interesting to me that if we maintain the position that all inline code is discouraged, then every field is isolated in its own component guarded by shouldComponentUpdate, which means the lion's share of the performance benefits of v6 are actually achievable in v5 without the separate connect calls for each field. Simply moving all field content into separate components so that the main form component "owns" nothing directly achieves most of the performance benefits.

Having content inside a function that is executed on every render is completely equivalent to having that same content directly inside the main form component, from a performance perspective.

For example, suppose you have this in your form:

Form = () => (
  <SomeStylingWidget>
    <p>Some extra arbitrary content goes here</p>
  </SomeStylingWidget>
)

Every time you rerender the form, you also have to rerender that widget content. In order to have better performance, you could move that widget stuff into a component guarded by shouldCompoentUpdate like this:

class WidgetContent extends Component {
  render() {
    <SomeStylingWidget>
      <p>Some extra arbitrary content goes here</p>
    </SomeStylingWidget>
  }
}

Form = () => (
  <WidgetContent />
)

However, one of advantages of react is that it efficiently resolves these differences for you so that you don't have to optimize things like this yourself in awkward ways.

First you optimize for the programmer, then you optimize for performance. In other words, we want to be able to write code that is the most clear and convenient, without being forced into tricks to achieve performance (at least until performance itself becomes the problem).

All of that said, expanding the API of a library has a lot of ramifications, so if you decide it's not the right time to add something like this, I completely understand.

[clayne11]

It's interesting to me that if we maintain the position that all inline code is discouraged, then every field is isolated in its own component guarded by shouldComponentUpdate, which means the lion's share of the performance benefits of v6 are actually achievable in v5 without the separate connect calls for each field. Simply moving all field content into separate components so that the main form component "owns" nothing directly achieves most of the performance benefits.

I think the issue with performance in v5 is that the form itself would re-render on every single change, thus re-rendering every component. Even if every component implemented shouldComponentUpdate, the form itself re-rendering becomes expensive if there are a lot of components inside of it.

Having content inside a function that is executed on every render is completely equivalent to having that same content directly inside the main form component, from a performance perspective.

The issue isn't with having content inside of a function - the issue is defining a new function on every render pass. That's expensive within the JavaScript VM itself, and you're removing an entire layer of optimizations done by the VM even without accounting for React optimizations.

That's my understanding of the Javascript VM at least. I'm happy to be proven wrong but that is one of my biggest issues with defining components inline, along with the fact that there's no reliable way to tell a function apart from a functional component as far as I know.

I would love to get @gaearon's opinion on this discussion.

[naw]

Yep I agree inline functions are slower from the VM standpoint, and I agree there is no reliable way to distinguish functions from functions intended as functional components, and I agree @gaearon's input would be welcome here. I agree about v5 and v6 too, which is why I said "most" and "lion's share", although which aspect is contributing most to better performance I could be wrong on.

[gaearon]

redux-form could just look at the function to see if there is a propTypes property, and if there is, it uses createElement(), otherwise it calls the function.

I don't think changing behavior depending on propTypes is a good solution. Some people remove them in production which would change the behavior between dev and prod.

[gaearon]

I think you could have a heuristic like this:

1. Create an element with component and props (this would trigger propTypes validation)
2. If component has a prototype, return that element
3. If component has contextTypes, return that element
4. Grab resolved props from the element (they would include defaultProps)
5. Get the result of calling component with resolved props
6. If the result is not null and has a render method, return the original element (React supports factory pattern where component returns { render } object)
7. Otherwise, return the result element

Not sure if it covers all cases but maybe it could work.

[erikras]

Thanks for that, @gaearon. That seems a little too close to the React metal than I would like. i.e. it feels like something that might break from a non-breaking release of React because it's sidestepping the public API.

Overall this has been a great discussion, but I haven't seen an argument that is so powerful as to overcome the inertia of leaving the API as is. Thank you to you all.

[lock]

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.