Support React 0.14

[gaearon]

No description provided.

[milankinen]

To be honest, I don't understand the design decisions of this pull request. Here are some thoughts:

Pure components(/functions) are pure: they don't need to be proxied. If I've understood right, the original idea behind the proxies was to preserve the internal state of React components across the component versions. But because the "pure components" don't have any internal state, there is (in my opinion) no need to create proxies for them - when the function implementation changes, the next re-render will apply the changes.

In addition, because pure components are just functions, detecting them programmatically (e.g. by using react-transform) is practically almost impossible. Or do you have any idea how to tell if the following one is a component or just a helper/utility function (okay, perhaps I little bit artificial example but I hope you understand the point)?

export default function() {
   const [val, defaultVal] = Array.prototype.slice.call(arguments)
   return val ? <h1>[val}<h1> : <h2>{defaultVal}</h2>
}

And finally, I don't see the removal of forceUpdate necessary. In my opinion, just returning an empty array for pure components would be better choice. The reason for this one is the actual reloading workflow:

1. When some changes occur, the changed module gets reloaded.
   1.1. If it contains "non-pure" components, then their instances can be detected, thus forceUpdate will do the job
   1.2 If it contains pure components, then they can't be detected and reloading will propagate to the parent module
2. Eventually reloading propagates to a (parent/ancestor) module containing "non-pure" components which get proxied and updated with forceUpdate. When this forceUpdate occurs, then also all pure child components (from child modules) get re-rendered with their new implementations.
3. If the whole application contains only pure components, reloading stops at the entry point of the bundle. But that entry point (at least) contains ReactDOM.render which gets re-evaluated and thus the all pure child components get re-rendered with their new implementations

Do these thoughts make any sense?

[gaearon]

I've been thinking about this for a month so believe me I did not make this change light hearted. 😉
As much as I don't like it, I think it's necessary.

Pure components(/functions) are pure: they don't need to be proxied. [...] because the "pure components" don't have any internal state, there is (in my opinion) no need to create proxies for them - when the function implementation changes, the next re-render will apply the changes.

If they're not proxied, this means we let their identities change, and let the changes bubble.
If we go that way we have several problems:

1. Change to a pure component results in DOM re-render of the whole tree below because React bails out of reconciliation. Not cool.
2. Worse yet, change to a pure component results in destroying the state of the whole tree below for the same reason. This means if you edit some pure component that is often used, you'll have different parts of the app resetting their state. That's something React Proxy was designed to avoid in the first place, as otherwise you could just as well let hot updates bubble to the top.
3. If we let the identity change on hot reload, we must force any file that exports a pure component to bubble the hot update, as otherwise nobody's going to reference it. This means we no longer can rely on “file with component is always accepted” being true. In fact this means that occasionally edits in deeply nested components will kill the state and DOM of the whole application just because they happened to propagate by the unfortunate path where every file had a function component export.

In addition, because pure components are just functions, detecting them programmatically (e.g. by using react-transform) is practically almost impossible.

I think the third point shows that “functions are hard to detect” is really isn't an argument in favor of doing nothing because, if we give up on detecting them, we might as well never accept() hot updates in component files (because we can't detect functions, and as we see in (3), it's essential because we have to propagate the hot update if there's at least one exported function).

But then if we have to always bubble hot updates up, any edit deep inside the tree will cause the whole tree to lose DOM and state just because there was a pure component somewhere along the way. So we get back to where we started.

Finally, there is module level error handling. If there's a module level error, Webpack is simply going to abort hot update (and any future hot update) if we don't handle it inside the module. Accepting hot update via module.hot.accept() inside the module is our single opportunity for doing so. If we give up on accepting hot updates inside the modules, we'll give up on this kind of error handling too. (This isn't a huge deal but it's still frustrating and a step back from what React Hot Loader offered.)

But to answer your question, yes, they are hard to detect, but we don't have to handle every case. We only need to handle the most common patterns, and document that.

Or do you have any idea how to tell if the following one is a component or just a helper/utility function (okay, perhaps I little bit artificial example but I hope you understand the point)?

export default function() {
  const [val, defaultVal] = Array.prototype.slice.call(arguments)
  return val ? <h1>[val}<h1> : <h2>{defaultVal}</h2>
}

Any pure function that returns a ReactElement, null, or false, is a component now. So technically yes, it is a component because it can be used like one. And nothing bad happens if React Proxy treats it like component.

Of course this is pushing it too far: for example, render() method of any class is technically a component, although one that crashes because it looks for this. But it doesn't mean we can't come up with heuristics that cover 95% use cases in the real world. For example, eslint-plugin-react is also going through tough times here (jsx-eslint/eslint-plugin-react#254, jsx-eslint/eslint-plugin-react#255, jsx-eslint/eslint-plugin-react#256, jsx-eslint/eslint-plugin-react#259) but I'm confident it will exhaust real-world false positives soon, and everyone will be productive again. A reasonable approximation of stateless component is a FunctionDeclaration or ArrowExpression with VariableAssignment parent in top level scope whose return clause contains either a JSX token or a createElement() call. This is good enough for most cases.

And finally, I don't see the removal of forceUpdate necessary. In my opinion, just returning an empty array for pure components would be better choice. The reason for this one is the actual reloading workflow

I hope I showed above why this workflow, as intuitive as it seems, does not achieve the goals of these projects (keeping state and DOM and doing our best to recover from errors). Returning an array of nodes gives the false idea that we support this tracking fully when it isn't true, and assumes that the consumer has some sort of bubbling mechanism available to them (which also often isn't true). Asking the consumer to register the root instance node themselves isn't a huge deal, and is much more reliable. In the future React will provide a special official reflection API for things like DevTools, which we'll use to grab the root instances—that's when we'll be able to remove this restriction, but not now.

Finally, we should all understand that React Proxy, React Transform, etc, are all temporary solutions. We are now experimenting with hot reloading and figuring out what on the React side makes it hard. There are some obvious pain points: state lives on instances (facebook/react#4594), no error boundaries (facebook/react#2461), no DevTools API (facebook/react#4593), etc. As these get solved, I hope that eventually we'll be able to move to a more natural approach like you described where hot updates “just bubble” and it still works. I know @sebmarkbage hates all the dirty business I'm doing. 😛 But I don't want to compromise on user experience, so I'm only ready to jump to a more “simple” solution when it's also a better one, and ideally officially supported on the React side, which I hope to work on in the future.

[gaearon]

By the way I have high hopes for module pattern:

https://github.com/reactjs/react-future/blob/master/07%20-%20Returning%20State/02%20-%20Module%20Pattern.js

https://github.com/reactjs/react-future/blob/master/09%20-%20Reduce%20State/01%20-%20Declarative%20Component%20Module.js

When React supports something of the sort, we won't have to worry about instances. Even preserving the identity will be much easier because module is just an object. Nothing like the function-proxying and prototype-patching business we've got going on right now.

So I'm hopeful for the future.

[andreypopp]

Regarding stateless component detection by inspecting AST there's a PR to react-docgen which might be useful here.

[remyrylan]

@gaearon Just to make sure I understand -- you're saying that you are intending to support stateless components with react-proxy, react-transform and react-transform-hmr? I ask because about 90% of my app could be stateless components, but I won't bother migrating if it's going to mean there won't be support for react-transform-hmr.

[milankinen]

Change to a pure component results in DOM re-render of the whole tree below because React bails out of reconciliation. Not cool.
...
Worse yet, change to a pure component results in destroying the state of the whole tree below

Well spotted, missed this one since I personally don't mix stateful and pure components in my sub-trees. I agree that this sub-tree re-rendering might be an issue.

This means we no longer can rely on “file with component is always accepted” being true.

We should not rely on that even at the moment (if I understood your statement right). We should always examine the exports because if we always accept the file that contains at least one component, we might miss references that are meaningful to our business logic:

// someUtils.js

// this one causes the module being accepted every time when the file changes?
class SomeUtilComponent extends React.Component {
  ...
}

export function renderUtils() {
   return isUtilEnabled() ? <SomeUtilComponent /> : <div>bal</div>
}

export function isUtilEnabled() {
  // how about if we make changes to this function?
  return !isIE()
}

If the pure components become mainstream, mixing utility code with components is likely to become more common. Then it's crucial to reload the references to "non-component" functions. Perhaps I just missed your point and you were meaning exactly same but just to make sure.

I think the third point shows that “functions are hard to detect” is really isn't an argument in favor of doing nothing because
...
Any pure function that returns a ReactElement, null, or false, is a component now. So technically yes, it is a component because it can be used like one. And nothing bad happens if React Proxy treats it like component.

I'm sorry I formulated my point unclearly. I agree with this once - something being hard to implement is not an excuse to not to implement it. My point is that if the pure component detection and proxying is applied, it must not cause false positives. Or at least if any false positives occur, the proxy implementation must not break the original behaviour.

Please bear me if I misread the current implementation but the following code would break the original implementation if the rule above is applied with the current implementation that loses function arity information (this is some code I could write, and I'd be quite pissed to change my implementation just for the sake of reloading)

// form.js
function Form(submitAction, props) {
   return ( /* ...render form... */ )
}

export default curry(Form)

// somewhereInApp.js
import From from "./form"
import {submitUserProfile} from "./actions"

const UserProfileForm = Form(submitUserProfile)

function MyComponent({userProfile}) {
  return (<some-jsx>...<UserProfileForm {...userProfile} />...</some-jsx>)
}

Asking the consumer to register the root instance node themselves isn't a huge deal, and is much more reliable.

I have to disagree with this one. As a reloading tool developers, we've failed if we need to ask the developer to add some extra code to the production codebase just to make reloading work. The more serious thing is that we have to expose react-proxy package in the codebase which should be 100% dev dependency only. Ok, tools like envify + uglifyify combo could solve this but I'd like to avoid this scenario (enforcing users to use such tools) at any cost.

The same "complain" (:smile:) partially applies to the existence of separate react-deep-force-update package. Please don't get me wrong, I'm a big fan of modularity but somewhere it crosses the line. The function is just ~100 LOC!! It doesn't need to be it's own module -- IMO there are even now a plenty of extra stuff to put into your package.json in order enable the hot loading.

Accepting hot update via module.hot.accept() inside the module is our single opportunity for doing so. If we give up on accepting hot updates inside the modules, we'll give up on this kind of error handling too.

I'm not familiar with internal error handling of Webpack HMR so I can't comment much about this one. 😄 But even now, there are cases when module.hot.accept() can't be applied directly to the modified module, right (e.g. when reloading redux reducers or action creators)? How does Webpack's error handling work with these cases?

[milankinen]

By the way I have high hopes for module pattern

Seems totally horribe and complex, especially the "declarative" component module example. I'll switch to cycle.js if that becomes mainstream. 😄

P.S. this(handleClick) wtf

P.P.S. Of course wonderful to HMR. But still the actual development aspect... Personally preferring a little bit poorer HMR capabilities over complex and harder-to-(read|understand) codebase.

[milankinen]

@JeremyRylan, correct. At the moment there are no working HMR implementations that support [email protected]

[gaearon]

@JeremyRylan

@gaearon Just to make sure I understand -- you're saying that you are intending to support stateless components with react-proxy, react-transform and react-transform-hmr? I ask because about 90% of my app could be stateless components, but I won't bother migrating if it's going to mean there won't be support for react-transform-hmr.

Sure, I intend to support them.

[gaearon]

I have to disagree with this one. As a reloading tool developers, we've failed if we need to ask the developer to add some extra code to the production codebase just to make reloading work. The more serious thing is that we have to expose react-proxy package in the codebase which should be 100% dev dependency only. Ok, tools like envify + uglifyify combo could solve this but I'd like to avoid this scenario (enforcing users to use such tools) at any cost.

I'm already asking people to do this for Redux DevTools so I don't really see the problem. React also explicitly tells people to envify their code to get the production version.

What I have in mind is this:

const instance = render(<App />, rootEl);

if (process.env.NODE_ENV !== 'production') {
  require('react-transform-hmr').addRootInstance(instance);
}

I don't see this as a big deal at all, and anyway, it's a temporary solution until React gets official DevTools API.

[gaearon]

Or at least if any false positives occur, the proxy implementation must not break the original behaviour.

I absolutely agree with this.
But preserving arity is doable if there is interest.
What else is there to get lost?

The same "complain" (:smile:) partially applies to the existence of separate react-deep-force-update package. Please don't get me wrong, I'm a big fan of modularity but somewhere it crosses the line. The function is just ~100 LOC!! It doesn't need to be it's own module -- IMO there are even now a plenty of extra stuff to put into your package.json in order enable the hot loading.

You're not supposed to use it directly. It's supposed to be a depedency of higher level tools like LiveReactload or react-transform-hmr. It needs to be separate because it has very specific set of tests and I want them to be independent of anything else to ease contribution in the future if React breaks it later.

[gaearon]

Seems totally horribe and complex, especially the "declarative" component module example. I'll switch to cycle.js if that becomes mainstream.

No need to attack straw man. These are “proposals” in an “experimental” repo precisely because this is what they are. Obviously if they were finished and sensible they wouldn't be there, and would be on track for implementation instead. 😉

But that's irrelevant to this thread anyway.

[milankinen]

But preserving arity is doable if there is interest.
What else is there to get lost?

It depends on the "weaver" implementation. If it replaces function declarations with constants then the evaluation order might be an issue:

// before
export default compose(doThis, doThat)

function doThis(args) {
   ...
}

function doThat(args) {
  ...
}

// after
export default compose(doThis, doThat)

const doThis = proxy(function doThis(args) { ... })

const doThat = proxy(function doThat(args) { ... })

[gaearon]

Good point. I think we should only wrap top-level const, let expressions, and exports to avoid messing with hoisting.