Support Functional Components

[ericclemmons]

🎈 🎉 Hot Module Replacement for Stateless Functional Components is (almost) here! 🎉 🎈

Having rewritten this logic 2 times already in ericclemmons/babel-plugin-react-pure-to-class#1, this makes my 3rd time :) I'm running on fumes, so I'm not interested in any major cleanup, restructure, or other chore tasks in this PR, simply resolving any blockers & letting the community reap the benefits.

This can be released as v2.1.0, but perhaps v3.0.0 is best because this is a substantial feature and possibly a BC break because there's no attainable way to verify compatibility with the ecosystem, IMO.

Stateless Functional Components

React v0.14 introduced them in this blog post:

https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#stateless-functional-components

var Aquarium = ({species}) => (
  <Tank>
    {getFish(species)}
  </Tank>
);

These are significantly cleaner in appearance, simplifies testing, and removes React-signatures making it possible to re-use JSX-style components with other VDOM implementations.

(I only mention this because i've been experimenting with shipping different VDOM builds to Mobile vs. Desktop users to save on bandwidth, but that's irrelevant to this PR.)

As such, it's encouraged to write SFCs as the roadblock of losing HMR is now removed.

Requirements

Because we have to rely on signatures to infer if a function is indeed a component, there are a few checks that have to pass:

• Component name must exist and must be capitalized (e.g. Foo, not foo or fooBar).
• Component must be top-level (e.g. no indentation or nesting). Otherwise, this is just annoying to implement.
• Component must return a JSX element. This is potentially the most controversial, but clearest signal IMO.
• React may be in scope. If not, import React from "react" is automatically inserted (for re-use & terseness, as indicated above).

Users, like myself, who'll enjoy automatic insertion of React into the scope should remove this ESLint rule:

diff --git a/.eslintrc b/.eslintrc
index da0e3ea..7a686c0 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -56,7 +56,7 @@
-    "react/react-in-jsx-scope": 1,
+    "react/react-in-jsx-scope": 0,

How it Works

1. Top-level expressions & variables are traversed.
2. If they are "React-like", they are converted into:

class ${name} extends React.Component {
  render() {
    ${variables}
    ${previousBody}
  }
}

1. react-transform continues working as usual!

Examples

Based on an impromptu Twitter survey, the following examples are supported (more in the fixture tests):

export const Foo = ({ ... }) => ( ... );

export default connect(Foo);

function Bar(props, context) {
  ...
}

export default Bar;

export default function Baz() { ... }`

const Bing = function() { ... };

Tests

• Hand-crafted fixtures are in test/fixtures/code-functional-* that match ideal scenarios.
• A few fixes were made after trying this out in several projects. Namely, enforcing the variable & function types (e.g. t.isArrowExpression, t.isIdentifier, etc.) to ensure only supported patterns are matched.
• Confirmed working in several work projects, many of which:
  • Use require.ensure with Webpack for code-splitting & lazy-loading of routes.
  • Leverage HMR already.
  • Contain a complex mix of statefull, stateless, @connected components, and even @resolved components.
  • Have propTypes set.

References

• First Release ericclemmons/babel-plugin-react-pure-to-class#1
• Support 0.14 pure components #34
• Support Functional Components #57

---

Alright, after all of this effort I'm spent.
🚬 🍺 🍰

[satya164]

\o/

cc @martinbigio :)

[jamiebuilds]

I guess I won't go into detail again about how bad of an idea this is. People are clearly in love with the syntax and don't give a fuck about the problems here, which is shortsighted. So I'll just say 👎 and unsubscribe.

[mik01aj]

@ericclemmons what exactly happens when the react-likeness-detection goes wrong? As far as I understand:

• when a function is deemed not react-like and it is used as a react component, it will work, just without hot reloading.
• when a function is deemed react-like and it is used as a function, it will break. (how?)

[inakianduaga]

I guess I won't go into detail again about how bad of an idea this is. People are clearly in love with the syntax and don't give a fuck about the problems here, which is shortsighted. So I'll just say 👎 and unsubscribe.

@thejameskyle Can you link to wherever you listed the cons of this approach? I'd like to know. Tx

[jasonslyvia]

So basically the way to enable functional components hot reload support is to transform them into class like React components?

[ericclemmons]

@mik01aj You're right on the money.

• If you have a functional component like <foo /> (rather than following conventions & using <Foo />), this PR will skip it and everything works as normal. No breaks, just manual reloading as usual.
• If you violate convention (e.g. http://eslint.org/docs/rules/new-cap) the way that express.Router does (a factory function that's capitalized), it can potentially cause a false positive.
• However, I forgot last night to list the other requirement that a function must return JSX to match!

That last rule I think will get rid of almost every false positive, unless you have something terrible like SomeComponentFactory that returns JSX.

That's why I advise either a major version bump or perhaps a transform option to opt-in to functional HMR.

[ericclemmons]

@inakianduaga He intentionally unsubscribed, so it'd be best to reach him through other channels, not yank him back into the thread :)

[ericclemmons]

@jasonslyvia That's correct. react-transform adds a lot value besides just HMR, as there are custom transformers to enhance errors, animate updates, etc.

This would be NODE_ENV="development"-only, so it'd mean you could opt-in to the majority of your components will be terse, clean functions and reap the benefits without incurring the LOC cost and using something like this:

https://github.com/thejameskyle/babel-plugin-react-pure-components

[mik01aj]

However, I forgot last night to list the other requirement that a function must return JSX to match!

You mean, you analyze all the return paths of a function?

[ericclemmons]

One thing that may be a good idea is to:

• Minor version bump, with the addition of requiring an additional option to enable this feature.
• This way, naysayers can keep quiet, while the rest of us who want to continue improving the DX get to do so.
• Then, revisit default options in a major version bump, as my informal polling shows that the majority of users of this lib copy/paste the options from README.md (not even knowing what locals is even referring to).

I'll let @gaearon weigh in, since this is his project and he's the one who encouraged me to send a PR rather than continue on ericclemmons/babel-plugin-react-pure-to-class#1.

[ericclemmons]

@mik01aj Actually, I analyze just the top-level return, of which there should only be one.

If there are other return paths, I don't look at them.

The ideal scenario is this:

const Greeting = ({ message }) => (
  <h1>Howdy there, {message}!</h1>
);

export default Greeting;

This removes almost all ambiguity & complexity in parsing.

If you had a terrible function like the one below, you'd lose out on HMR, but in no way can this be bulletproof :)

const Greeting = ({ message }) => {
  if (message) {
    return <h1>Howdy there, {message}!</h1>;
  }

  return false;
}

Should this be merged in, there'd be an explicit recommendation in the README.md to match the most common pattern people are already using.

[ericclemmons]

Great constructive feedback so far (mostly :D). Tests should be passing in CI shortly...

[ericclemmons]

Sweet, CI is passing and I improved the README.md for how to best leverage it:

https://github.com/ericclemmons/babel-plugin-react-transform/blob/57-functional-components/README.md#stateless-functional-components

[gaearon]

I thought about this some more and I remembered why I didn’t favor “convert ’em to classes” approach. There is an important correctness issue here. Namely, React doesn’t let functional components get refs. However this would technically allow those components to have refs in development. You can rely on this, and it will break in production.

I don’t see any good way around this. The only implementation that wouldn’t suffer from this problem (IMO) would be the one that kept functions as they are, and passed them to the transforms. Of course this would require changes in transforms themselves (so they can handle functions) but I don’t think this is such a big deal because we only have a couple of popular transforms, and they will be easy to fix.

Thoughts?

[ericclemmons]

Hmm, I dunno. I'm not 100% sure what this line means:

These components behave just like a React class with only a render method defined. Since no component instance is created for a functional component, any ref added to one will evaluate to null. Functional components do not have lifecycle methods, but you can set .propTypes and .defaultProps as properties on the function.

It sounds like this is referring to the owner's render method:

// ./Greeting.js
//   - This would become a Class
const Greeting = ({ name }) => (<h1>Howdy there, {name}!</h1>);

export default class App extends React.Component {
  render() {
    // Based on the blog post, this.refs.greeting = null, but after the transform,
    // this.refs.greeting = (instanceof Greeting).
    return <Greeting name="Dan" ref="greeting" />;
  }
}

Frankly, I dunno. I'm largely spent on this and the window of opportunity is closing for me.

I don't see a good way around it either, besides:

• Rewrite transforms to support functions.
• Instead of replacing the functions with classes, put their class in the scope as unique variable and have the function render that instead. If this works, it would mean consuming classes would still have this.refs.greeting = null, but the functional body has been converted into a HMR-friendly class.

class _Greeting12387 extends React.Component {
  render() {
    const { name } = this.props;
    return <h1>Howdy there, {name}!</h1>
  }
}

const Greeting = (args) => <_Greeting12387 {...args} />

• Say "pffft, let's just warn the user in the docs" and figure it out later.

Let me know what you think. The hacker in me says "We can figure this out!", but the full-time employee in me says, "My new React devs could really use this now" :D

[gaearon]

Frankly, I dunno. I'm largely spent on this and the window of opportunity is closing for me.

I’m sorry I didn’t mention this before. It’s something I had in the back of my mind when I decided not to pursue the “converting” approach but I didn’t really put my thoughts out in public.

Instead of replacing the functions with classes, put their class in the scope as unique variable and have the function render that instead. If this works, it would mean consuming classes would still have this.refs.greeting = null, but the functional body has been converted into a HMR-friendly class.

This would work but the downside is that every component would effectively become two components from React’s point of view. This would make performance even worse than it is now, and make DevTools view confusing and annoying.

Say "pffft, let's just warn the user in the docs" and figure it out later.

There is no centralized docs for this, and that is the problem. There are boilerplate projects and example projects using this plugin with a few transforms, sometimes as a preset. Wherever we put this, people most likely to hit this will not read it.

Since we’ve been encouraging them to put the transform behind a dev flag, we can’t allow them to write code that works in dev and breaks in prod. Even the opposite is not as bad.

I’m afraid this is a really big drawback of this approach. I really appreciate your effort and I’m sorry I didn’t realize this was a problem sooner. I think that the right way forward would be:

Rewrite transforms to support functions.

This would also make your implementation simpler because you wouldn’t need to transform them this much—only to wrap them. And it is trivial to adjust react-transform-hmr and react-transform-catch-error to support functions. I can take care of this.

[ericclemmons]

The functionToClass function & it's dependencies can come out pretty easily, so no problem there.

Worst-case scenario (we're about to start up a new project that could benefit from this), I'll create a babel-plugin-preset-functional-hmre in the interim from this fork.

Let me know how it goes and when this can be updated.

[lostfictions]

@ericclemmons Did that fork happen, by any chance? I understand the caveats and was interested in giving this a try.

(I tried setting it up in my project by removing babel-preset-react-hmre from my package.json and specifying its plugins explicitly instead, with babel-plugin-react-transform's version set to ericclemmons/babel-plugin-react-transform#57-functional-components -- but it doesn't seem to work. Maybe I missed a step?)

[SpenserJ]

I've made a commit in my forks of babel-plugin-react-transform and react-transform-catch-errors that adds in basic support for hot-reloading stateless functional components, as well as catching errors in them. It needs a bit more work (lacking tests and support for function(props) { return <div />; }), but it might work for you @lostfictions

[tonyxiao]

@gaearon is it time to update the official recommendation in this repo considering this RFC in redux? #85

[gaearon]

I still want to add support for them here—just not by converting them to classes.
If anyone is willing to contribute that support, I’m up for merging it.

[ericclemmons]

@gaearon I was waiting on you:

This would also make your implementation simpler because you wouldn’t need to transform them this much—only to wrap them. And it is trivial to adjust react-transform-hmr and react-transform-catch-error to support functions. I can take care of this.

[gaearon]

Oh, I’m sorry, I should’ve made it clear I didn’t intend to work on this right away 😞 .

Realistically I expect to have time to work on this in a month or two.
If you still have the drive to make it happen before, please go for it!

[gaearon]

I will close this PR per reasons above. Happy to see another one dealing with functions directly.

[ericclemmons]

For the rest following the thread, I will likely create a functional equivalent of https://github.com/danmartinez101/babel-preset-react-hmre for those of us that don't have 100% of our component state abstracted into a global.

[gaearon]

for those of us that don't have 100% of our component state abstracted into a global.

Not sure if this is snark 😉

As I said before I’m totally on board with supporting functional components in a way that does not introduce potential null references in the production that you don’t have in development. Because null references are bad and I don’t trust myself to remember which components are functional and which aren’t. It’s not good if a developer can refactor a class component to be functional, use findDOMNode(stuff) somewhere on it, and still have it work in dev due to a build tool quirk, only to fail in production.

My decision to close this PR has nothing to do with Redux or single state atom, and everything to do with the fragility of approach you are suggesting. Having issues in production because of hot reloading is going to harm its credibility.

As I said before I’m happy to accept a PR that implements it in a reasonably safe way by wrapping functions instead of turning them into classes.

[ericclemmons]

Wasn't snark. I understand the reasoning, I was just updating the thread
because I know I'm not the only one in this boat that still wants it, warts
& all.

On Sun, Mar 6, 2016 at 7:17 PM Dan Abramov notifications@github.com wrote:

for those of us that don't have 100% of our component state abstracted
into a global.

Not sure if this is snark [image: 😉]

As I said before I’m totally on board with supporting functional
components in a way that does not introduce potential null references in
the production that you don’t have in development. Because null
references are bad and I don’t trust myself to remember which components
are functional and which aren’t. It’s not good if a developer can refactor
a class component to be functional, use findDOMNode(stuff) somewhere on
it, and still have it work in dev due to a build tool quirk, only to fail
in production.

My decision to close this PR has nothing to do with Redux or single state
atom, and everything to do with the fragility of approach you are
suggesting. Having issues in production because of hot reloading is going
to harm its credibility.

As I said before I’m happy to accept a PR that implements it in a
reasonably safe way by wrapping functions instead of turning them into
classes.

—
Reply to this email directly or view it on GitHub
#85 (comment)
.

• Eric Clemmons

[gaearon]

React Hot Loader 3 supports functional components without destroying the state.
It supersedes both React Hot Loader and React Transform, and is built on lessons learned from both.
Check it out: gaearon/react-hot-boilerplate#61

[ericclemmons]

This is a better, long awaited solution.

The problem for myself (and likely many others) is that I have no idea what needs to be done for Webpack to know how to hot-reload something on the client (without doing a browser reload).

[gaearon]

Does https://medium.com/@dan_abramov/hot-reloading-in-react-1140438583bf help?