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

Optimizing Compiler: Reuse Constant Value Types like ReactElement #3226

Closed
sebmarkbage opened this Issue Feb 22, 2015 · 34 comments

Comments

Projects
None yet
@sebmarkbage
Member

sebmarkbage commented Feb 22, 2015

Starting with 0.14 we will be able to start treating ReactElements and their props objects as value types. I.e. any instance is conceptually equivalent if all their values are the same. This allow us to reuse any ReactElement whose inputs are deeply immutable or effectively constant.

Take this function for example:

function render() {
   return <div className="foo" />;
}

This can be optimized by moving the JSX out of the function body so that each time it is called the same instance is returned:

var foo = <div className="foo" />;
function render() {
   return foo;
}

Not only does it allow us to reuse the same objects, React will automatically bail out any reconciliation of constant components - without a manual shouldComponentUpdate.

Reference Equality

Objects in JavaScript have reference equality. Meaning that this optimization can actually change behavior of code. If any of your calls to render() uses object equality or uses the ReactElement as the key in a Map, this optimization will break that use case. So don't rely on that.

This is a change in the semantic contract of ReactElements. This is difficult to enforce, but hopefully a future version of JavaScript will have the notion of value equality for custom objects so this can be enforced.

What is Constant?

The simplest assumption is if the entire expression, including all the props (and children), are all literal value types (strings, booleans, null, undefined or JSX), then the result is constant.

function render() {
  return <div className="foo"><input type="checkbox" checked={true} /></div>;
}

If a variable is used in the expression, then you must first ensure that it is not ever mutated since then the timing can affect the behavior.

var Foo = require('Foo');
function createComponent(text) {
  return function render() {
    return <Foo>{text}</Foo>;
  };
}

It is safe to move a constant to a higher closure if the variable is never mutated. You can only move it to a scope that is shared by all variables.

var Foo = require('Foo');
function createComponent(text) {
  var foo = <Foo>{text}</Foo>;
  return function render() {
    return foo;
  };
}

Are Objects Constant?

Arbitrary objects are not considered constant. A transpiler should NEVER move a ReactElement scope if any of the parameters is a mutable object. React will silently ignore updates and it will change behavior.

function render() {
  return <div style={{ width: 100 }} />; // Not safe to reuse...
}
// ...because I might do:
render().props.style.width = 200;
expect(render().props.style.width).toBe(100);

If an object is provably deeply immutable (or effectively immutable by never being mutated), the transpiler may only move it to the scope where the object was created or received.

function render() {
  var style = Object.freeze({ __proto__: null, width: 100 });
  return <div style={style} />; // Not safe to reuse...
}
// ...because I might do:
expect(render().props.style).not.toBe(render().props.style);

// However this is...
function createComponent(width) {
  var style = Object.freeze({ __proto__: null, width: +width });
  return function render() {
    return <div style={style} />; // ...safe to move this one level up
  };
}

This is due to the fact that arbitrary objects have referential identity in JavaScript. However, if the semantics of an immutable object is expected to have value equality, it might be ok to treat them as value types. For example any data structure created by immutable-js may be treated as a value type if it is deeply immutable.

Exception: ref="string"

There is unfortunately one exception. If the ref prop might potentially might have a string value. Then it is never safe to reuse the element. This is due to the fact that we capture the React owner at the time of creation. This is an unfortunate artifact and we're looking at various options of changing the refs semantics to fix it.

render() {
  // Neither of these...
  return <div ref="str" />;
  // ...are safe to reuse...
  return <div ref={possibleStringValue} />;
  // ...because they might contain a ref.
  return <div {...objectThatMightContainARef} />;
}

Non-JSX

This can work on JSX, React.createElement or functions created by React.createFactory.

For example, it is safe to assume that this function call generates a constant ReactElement.

var Foo = React.createFactory(FooClass);

function render() {
  return Foo({ bar: 1 });
}

Therefore it is safe to reuse:

var Foo = React.createFactory(FooClass);
var foo = Foo({ bar: 1 }};
function render() {
  return foo;
}

Advanced Optimizations

You can also imagine even more clever optimizations that optimize per-instance elements by memoizing it on the instance. This allows auto-bound methods to be treated as effectively constant.

If you can track pure-functions, you can even treat calculated values as constants if the input to the pure function is constant.

Static analysis tools like Flow makes it possible to detect that even more elements are constant.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
Member

sebmarkbage commented Feb 22, 2015

cc @sebmck

@sebmarkbage sebmarkbage changed the title from Optimizing Compiler for Constant Value Types like ReactElement to Optimizing Compiler: Reuse Constant Value Types like ReactElement Feb 22, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 22, 2015

Member

This is great and exactly the type of thing I had in mind with babel/babel#653. Babel has pretty strong scope and reassignment tracking so these types of optimisations should be relatively straightforward.

Member

kittens commented Feb 22, 2015

This is great and exactly the type of thing I had in mind with babel/babel#653. Babel has pretty strong scope and reassignment tracking so these types of optimisations should be relatively straightforward.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 22, 2015

Member

If the optimization in #3228 is applied, then the children array and the props object itself might be reused independently from the element itself.

render() {
  return <div className={this.props.className}>hello <span>world</span></div>;
}
var children = ['hello', { type: 'span', props: { children: 'world' }, key: null, ref: null }];
render() {
  return { type: 'div', props: { className: this.props.className, children: children }, key: null, ref: null };
}

This can be useful for reusing empty props objects:

render(someKey) {
  return <Foo key={someKey} />;
}
var emptyProps = {};
render(someKey) {
  return { type: 'div', props: emptyProps, key: someKey, ref: null };
}
Member

sebmarkbage commented Feb 22, 2015

If the optimization in #3228 is applied, then the children array and the props object itself might be reused independently from the element itself.

render() {
  return <div className={this.props.className}>hello <span>world</span></div>;
}
var children = ['hello', { type: 'span', props: { children: 'world' }, key: null, ref: null }];
render() {
  return { type: 'div', props: { className: this.props.className, children: children }, key: null, ref: null };
}

This can be useful for reusing empty props objects:

render(someKey) {
  return <Foo key={someKey} />;
}
var emptyProps = {};
render(someKey) {
  return { type: 'div', props: emptyProps, key: someKey, ref: null };
}
@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Mar 6, 2015

Member

Did some initial prototyping in babel/babel@2703338. Pretty rough and there are quite a few edgecases I can think of that it doesn't handle but it's a good start.

function render() {
  return <div className="foo" />;
}

becomes

"use strict";

var _ref = React.createElement("div", { className: "foo" });

function render() {
  return _ref;
}

and

var Foo = require('Foo');
function createComponent(text) {
  return function render() {
    return <Foo>{text}</Foo>;
  };
}

becomes

"use strict";

var Foo = require("Foo");
function createComponent(text) {
  var _ref = React.createElement(Foo, null, text);

  return function render() {
    return _ref;
  };
}
Member

kittens commented Mar 6, 2015

Did some initial prototyping in babel/babel@2703338. Pretty rough and there are quite a few edgecases I can think of that it doesn't handle but it's a good start.

function render() {
  return <div className="foo" />;
}

becomes

"use strict";

var _ref = React.createElement("div", { className: "foo" });

function render() {
  return _ref;
}

and

var Foo = require('Foo');
function createComponent(text) {
  return function render() {
    return <Foo>{text}</Foo>;
  };
}

becomes

"use strict";

var Foo = require("Foo");
function createComponent(text) {
  var _ref = React.createElement(Foo, null, text);

  return function render() {
    return _ref;
  };
}
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 6, 2015

Member

nice!

Member

sebmarkbage commented Mar 6, 2015

nice!

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 6, 2015

Contributor

This is an optimization that affects run-time behavior though, right?

Contributor

syranide commented Mar 6, 2015

This is an optimization that affects run-time behavior though, right?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 6, 2015

Member

That is only because we don't have the distinction between value equality and physical equality in JavaScript. That's what we're trying to change. https://github.com/sebmarkbage/ecmascript-immutable-data-structures

You're very unlikely to rely on different referential equality of two otherwise equivalent React Elements. I've never seen a case for it, unless you have a mutable nested object. In which case this optimization should not apply.

Member

sebmarkbage commented Mar 6, 2015

That is only because we don't have the distinction between value equality and physical equality in JavaScript. That's what we're trying to change. https://github.com/sebmarkbage/ecmascript-immutable-data-structures

You're very unlikely to rely on different referential equality of two otherwise equivalent React Elements. I've never seen a case for it, unless you have a mutable nested object. In which case this optimization should not apply.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 6, 2015

Member

This is an optimization that should ideally be a applied in development mode too, just to be safe.

Member

sebmarkbage commented Mar 6, 2015

This is an optimization that should ideally be a applied in development mode too, just to be safe.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 8, 2015

Member

I realized that if you have the code:

var CurrentTime = React.createClass({
  render: function() {
    return <div>{"" + new Date()}</div>;
  }
})

var Clock = React.createClass({
  componentDidMount: function() {
    this.interval = setInterval(() => this.forceUpdate(), 1000);
  },
  componentWillUnmount: function() {
    clearInterval(this.interval);
  },
  render: function() {
    return (
      <div>Right now, its: <CurrentTime /></div>
    );
  }
});

React.render(<Clock />, document.body);

then currently this shows an autoupdating time, but would not with this optimization. :\

Wonder if we can come up with some way to catch this in development (i.e., look for DOM mutations that happen?) and warn or something.

Member

sophiebits commented Mar 8, 2015

I realized that if you have the code:

var CurrentTime = React.createClass({
  render: function() {
    return <div>{"" + new Date()}</div>;
  }
})

var Clock = React.createClass({
  componentDidMount: function() {
    this.interval = setInterval(() => this.forceUpdate(), 1000);
  },
  componentWillUnmount: function() {
    clearInterval(this.interval);
  },
  render: function() {
    return (
      <div>Right now, its: <CurrentTime /></div>
    );
  }
});

React.render(<Clock />, document.body);

then currently this shows an autoupdating time, but would not with this optimization. :\

Wonder if we can come up with some way to catch this in development (i.e., look for DOM mutations that happen?) and warn or something.

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Mar 10, 2015

Member

@spicyj : I think the definition of constant values has to be that the expression contains zero references to non-local bindings and zero side-effectful expressions (member expressions, function calls, etc).

So with that assumption, we're fine here because <div>Right now, it's: <CurrentTime /></div> always evaluates to a constant (it doesn't eagerly execute the CurrentTime render() function -- just a descriptor).

Member

jeffmo commented Mar 10, 2015

@spicyj : I think the definition of constant values has to be that the expression contains zero references to non-local bindings and zero side-effectful expressions (member expressions, function calls, etc).

So with that assumption, we're fine here because <div>Right now, it's: <CurrentTime /></div> always evaluates to a constant (it doesn't eagerly execute the CurrentTime render() function -- just a descriptor).

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 10, 2015

Member

No, @spicyj is right because of how the reconciliation bails out by default if the element is reused.

However, relying on global state without a forceUpdate is not a supported use case. The solution is to add a forceUpdate on a timer in the deeper component.

On Mar 10, 2015, at 2:50 PM, Jeff Morrison notifications@github.com wrote:

@spicyj : I think the definition of constant values has to be that the expression contains zero references to non-local bindings and zero side-effectful expressions (member expressions, function calls, etc).

So with that assumption, we're fine here because

Right now, it's:
always evaluates to a constant (it doesn't eagerly execute the CurrentTime render() function -- just a descriptor).


Reply to this email directly or view it on GitHub.

Member

sebmarkbage commented Mar 10, 2015

No, @spicyj is right because of how the reconciliation bails out by default if the element is reused.

However, relying on global state without a forceUpdate is not a supported use case. The solution is to add a forceUpdate on a timer in the deeper component.

On Mar 10, 2015, at 2:50 PM, Jeff Morrison notifications@github.com wrote:

@spicyj : I think the definition of constant values has to be that the expression contains zero references to non-local bindings and zero side-effectful expressions (member expressions, function calls, etc).

So with that assumption, we're fine here because

Right now, it's:
always evaluates to a constant (it doesn't eagerly execute the CurrentTime render() function -- just a descriptor).


Reply to this email directly or view it on GitHub.

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Mar 10, 2015

Member

I guess the difference is that before, if you didn't do a forceUpdate() (i.e. you didn't comply) you'd just lose out on a re-render...whereas with this optimization, you could actually be breaking more than just UI rendering

Member

jeffmo commented Mar 10, 2015

I guess the difference is that before, if you didn't do a forceUpdate() (i.e. you didn't comply) you'd just lose out on a re-render...whereas with this optimization, you could actually be breaking more than just UI rendering

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 10, 2015

Member

Well, it is still just UI rendering, but yea, this is an added risk. We make the following two assumptions:

  1. ReactElement is a value type. (including the class itself)
  2. render() is idempotent. We always called it a pure function of props and state.

We don't have have runtime or static type system support for validating those things yet. We have to rely on convention to enforce those constraints. If you violate those, you're SOL.

However, this optimization is not the only place you'll risk getting screwed. It is easy to shoot yourself in the foot in other places too if you violate these principals.

We'll just have to make harder to bring value types and pure functions to the runtime and Flow to make it easier to ensure that you're compliant. No need to punish compliant code while we wait.

We've had this idea of doing pseudo-random double rendering in __DEV__ so that we can validate that renders are idempotent.

Member

sebmarkbage commented Mar 10, 2015

Well, it is still just UI rendering, but yea, this is an added risk. We make the following two assumptions:

  1. ReactElement is a value type. (including the class itself)
  2. render() is idempotent. We always called it a pure function of props and state.

We don't have have runtime or static type system support for validating those things yet. We have to rely on convention to enforce those constraints. If you violate those, you're SOL.

However, this optimization is not the only place you'll risk getting screwed. It is easy to shoot yourself in the foot in other places too if you violate these principals.

We'll just have to make harder to bring value types and pure functions to the runtime and Flow to make it easier to ensure that you're compliant. No need to punish compliant code while we wait.

We've had this idea of doing pseudo-random double rendering in __DEV__ so that we can validate that renders are idempotent.

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Mar 10, 2015

Contributor

I'd like to propose moving the initialization for static elements into either componentWillMount or the constructor, and caching them on the module level. Taking out all of the static elements of a huge React app would add a significant initialization time overhead and also initialize components that might not be rendered at all (imagine downloading the full JS for the App but only rendering one view at a time).

I propose doing this optimization for React.createClass and classes that extend React.Component:

// before
class MyComponent extends React.Component {
  render: function() {
    return <div className="foo" />;
  }
}

// after
var _ref;
class MyComponent extends React.Component {
  constructor(props, context) {
    super(props, context),

    _ref = _ref || React.createElement("div", { className: "foo" });
  },
  render: function() {
    return _ref;
  }
}

Similarly, for a React.createClass call it would inject this into componentWillMount or create componentWillMount.

Contributor

cpojer commented Mar 10, 2015

I'd like to propose moving the initialization for static elements into either componentWillMount or the constructor, and caching them on the module level. Taking out all of the static elements of a huge React app would add a significant initialization time overhead and also initialize components that might not be rendered at all (imagine downloading the full JS for the App but only rendering one view at a time).

I propose doing this optimization for React.createClass and classes that extend React.Component:

// before
class MyComponent extends React.Component {
  render: function() {
    return <div className="foo" />;
  }
}

// after
var _ref;
class MyComponent extends React.Component {
  constructor(props, context) {
    super(props, context),

    _ref = _ref || React.createElement("div", { className: "foo" });
  },
  render: function() {
    return _ref;
  }
}

Similarly, for a React.createClass call it would inject this into componentWillMount or create componentWillMount.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 10, 2015

Contributor

@cpojer Should probably make it _ref = _ref || ....

Contributor

syranide commented Mar 10, 2015

@cpojer Should probably make it _ref = _ref || ....

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Mar 10, 2015

Contributor

oh yes, of course! Edited my example.

It could also of course have a single check for all of the static components in one module, like var _initialized = false; and an if-statement.

Contributor

cpojer commented Mar 10, 2015

oh yes, of course! Edited my example.

It could also of course have a single check for all of the static components in one module, like var _initialized = false; and an if-statement.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 10, 2015

Member

A simpler transform would be to do

var _ref;
class MyComponent extends React.Component {
  render: function() {
    return _ref || (_ref = <div className="foo" />);
  }
}

which might be a little slower than your proposal but much easier to transform.

Member

sophiebits commented Mar 10, 2015

A simpler transform would be to do

var _ref;
class MyComponent extends React.Component {
  render: function() {
    return _ref || (_ref = <div className="foo" />);
  }
}

which might be a little slower than your proposal but much easier to transform.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 10, 2015

Member

@cpojer That is a much more specific optimization that ties it specifically to assumptions about React classes. I don't think we're ready for that yet. See Advanced Optimizations for other use cases.

This optimization is really a generic one that is employed by functional engines to any value type all over. It is also generalizable to the value types proposal for ES7.

We should really hold off on the React specific ones. I'd also argue that you don't need your React class if you don't intend to call render on it yet. So the whole module and class can be deferred.

That's a much more generic optimization that can be used for a lot of things. But it separate from this issue.

Member

sebmarkbage commented Mar 10, 2015

@cpojer That is a much more specific optimization that ties it specifically to assumptions about React classes. I don't think we're ready for that yet. See Advanced Optimizations for other use cases.

This optimization is really a generic one that is employed by functional engines to any value type all over. It is also generalizable to the value types proposal for ES7.

We should really hold off on the React specific ones. I'd also argue that you don't need your React class if you don't intend to call render on it yet. So the whole module and class can be deferred.

That's a much more generic optimization that can be used for a lot of things. But it separate from this issue.

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Mar 10, 2015

Contributor

Is there a reason however not to include this in the transform? I don't think it would make the transform that much harder to build – I'm not proposing only allowing the general optimization for React components, but have a separate optimization that makes this work better for React components.

Contributor

cpojer commented Mar 10, 2015

Is there a reason however not to include this in the transform? I don't think it would make the transform that much harder to build – I'm not proposing only allowing the general optimization for React components, but have a separate optimization that makes this work better for React components.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 11, 2015

Member

@cpojer It ties it to subtle differences in React semantics that we can't prove are sound. For example, MyComponent.prototype.render() wouldn't work. A sub-class that doesn't call the super constructor wouldn't work. Etc. @spicyj's suggestion would be safer but comes with a performance penalty.

Besides, you have to allocate the binding slot for the variable regardless.

I think we're better off investing in more generic optimizations such as lazy initialization of the entire module body. If we need statics, then we can pull them apart from the main classes.

For example:

// foo.js
export class Foo {
  render() {
  }
}
Foo.myConst = 10;
// bar.js
import { Foo } from "foo";
now(Foo.myConst);
function later() {
  new Foo();
}

Could be transformed into (something equivalent to):

// foo-static-content.js
export myConst = 10;
// foo-deferred-content.js
export class Foo {
  render() {
  }
}
// bar.js
import { myConst } from "foo-static-content";
now(myConst);
function later() {
  new (require("foo-deferred-content"))();
}
Member

sebmarkbage commented Mar 11, 2015

@cpojer It ties it to subtle differences in React semantics that we can't prove are sound. For example, MyComponent.prototype.render() wouldn't work. A sub-class that doesn't call the super constructor wouldn't work. Etc. @spicyj's suggestion would be safer but comes with a performance penalty.

Besides, you have to allocate the binding slot for the variable regardless.

I think we're better off investing in more generic optimizations such as lazy initialization of the entire module body. If we need statics, then we can pull them apart from the main classes.

For example:

// foo.js
export class Foo {
  render() {
  }
}
Foo.myConst = 10;
// bar.js
import { Foo } from "foo";
now(Foo.myConst);
function later() {
  new Foo();
}

Could be transformed into (something equivalent to):

// foo-static-content.js
export myConst = 10;
// foo-deferred-content.js
export class Foo {
  render() {
  }
}
// bar.js
import { myConst } from "foo-static-content";
now(myConst);
function later() {
  new (require("foo-deferred-content"))();
}

kittens added a commit to babel/babel that referenced this issue Mar 29, 2015

add a path hoisting mechanism that will hoist a node to it's highest …
…compatible scope, a compatible scope is considered to be one where all references inside can be resolved to, also adds an optimisation.react.constantElements transformer that uses this to much success facebook/react#3226
@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Mar 29, 2015

Member

I've done a complete and stable implementation of this (see attached commit). Still need to add a lot more tests so I can be absolutely confident in it's reliability.

Member

kittens commented Mar 29, 2015

I've done a complete and stable implementation of this (see attached commit). Still need to add a lot more tests so I can be absolutely confident in it's reliability.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens May 16, 2015

Member

Just an update but the transformer is now smarter in it's hoisting and no longer deopts completely on reassigned bindings.

For example:

function renderName(firstName) {
  firstName += " (inactive)";
  return function () {
    return <span>{firstName}</span>;
  };
}

becomes

"use strict";

function renderName(firstName) {
  firstName += " (inactive)";

  var _ref = React.createElement(
    "span",
    null,
    firstName
  );

  return function () {
    return _ref;
  };
}

and no longer bails because of firstName +=.

Member

kittens commented May 16, 2015

Just an update but the transformer is now smarter in it's hoisting and no longer deopts completely on reassigned bindings.

For example:

function renderName(firstName) {
  firstName += " (inactive)";
  return function () {
    return <span>{firstName}</span>;
  };
}

becomes

"use strict";

function renderName(firstName) {
  firstName += " (inactive)";

  var _ref = React.createElement(
    "span",
    null,
    firstName
  );

  return function () {
    return _ref;
  };
}

and no longer bails because of firstName +=.

@mschipperheyn

This comment has been minimized.

Show comment
Hide comment
@mschipperheyn

mschipperheyn Jun 26, 2015

What about language keys? Let's say we have a multilingual application that calls these kinds of elements a lot

<FormattedMessage message="ui.login"/>

We could move all of these calls out of the render method because a language chosen is generally constant. Unless the user wants to change language. In that case you would like a one time invalidation of these constants. Would something like that be conceivable?

What about language keys? Let's say we have a multilingual application that calls these kinds of elements a lot

<FormattedMessage message="ui.login"/>

We could move all of these calls out of the render method because a language chosen is generally constant. Unless the user wants to change language. In that case you would like a one time invalidation of these constants. Would something like that be conceivable?

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Jun 26, 2015

Member

That element is already considered a constant JSX element so will be hoisted.

Member

kittens commented Jun 26, 2015

That element is already considered a constant JSX element so will be hoisted.

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Sep 3, 2015

Contributor

@sebmarkbage
Do you have any updates on the issue?

Contributor

koba04 commented Sep 3, 2015

@sebmarkbage
Do you have any updates on the issue?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 23, 2015

Member

Babel supports this in conjunction with React 0.14. Enable the optimisation.react.constantElements transform (do so in production only, e.g., when minifying your code).

Member

sophiebits commented Sep 23, 2015

Babel supports this in conjunction with React 0.14. Enable the optimisation.react.constantElements transform (do so in production only, e.g., when minifying your code).

@sophiebits sophiebits closed this Sep 23, 2015

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Feb 7, 2016

Contributor

Is this transformation potentially dangerous?

Contributor

chicoxyzzy commented Feb 7, 2016

Is this transformation potentially dangerous?

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 7, 2016

Member

What happens when you try it?

Member

kittens commented Feb 7, 2016

What happens when you try it?

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Feb 7, 2016

Contributor

I was deceived that transformer can hoist constant element when it shouldn't but after some time playing with my dispute opponent's examples I realised transform works properly. Sorry for false alarm =)

Contributor

chicoxyzzy commented Feb 7, 2016

I was deceived that transformer can hoist constant element when it shouldn't but after some time playing with my dispute opponent's examples I realised transform works properly. Sorry for false alarm =)

TimMensch pushed a commit to TimMensch/buble that referenced this issue Oct 31, 2016

Merge branch 'jsx' into 'master'
JSX support

By popular demand...

This implements very basic support for JSX. Still to do:

* Support pragmas other than `React.createElement`
* Optimisations (see [here](https://medium.com/doctolib-engineering/improve-react-performance-with-babel-16f1becfaa25#.thsp2ymcd) and [here](facebook/react#3226))
* Some method to auto-import needed modules? (e.g. automatically add `import * as React from 'react'`

It's still an open question whether this truly belongs in core, but I do like the convenience of it.

Since I don't ever use JSX, I could have completely pooched this up – would welcome input from people more experienced with it.

See merge request !37
@msuperina

This comment has been minimized.

Show comment
Hide comment
@msuperina

msuperina Mar 28, 2017

I see this issue is closed but cannot find a better place to post this. I have developed a small module https://www.npmjs.com/package/react-cache which handles caching React elements with variable props. It is a declarative way to replace shouldComponentUpdate for now.
At the moment I am working on an improvement to automatically detect during the transpilation phase what the cache function should be.

msuperina commented Mar 28, 2017

I see this issue is closed but cannot find a better place to post this. I have developed a small module https://www.npmjs.com/package/react-cache which handles caching React elements with variable props. It is a declarative way to replace shouldComponentUpdate for now.
At the moment I am working on an improvement to automatically detect during the transpilation phase what the cache function should be.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 4, 2017

Member

I think @sophiebits’s example in #3226 (comment) also presents a less obvious place where it could break:

class Parent extends React.Component {
  state = { person: { name: 'Dan' } };

  cachedEl = <Child person={this.state.person} />;

  handleClick = () => {
    this.state.person.name = 'Dominic';
    this.forceUpdate(); // Or even this.setState({})
  }

  render() {
    return (
      <div onClick={this.handleClick}>
        {this.cachedEl}
        <Child person={this.state.person} />
      </div>
    );
  }
}

function Child(props) {
  return <h1>Hi, {props.name}!</h1>;
}

In this example, one Child will get updated but the other one doesn’t, even though Child did “nothing wrong”. It didn’t depend on any external data. It just happened to accept a prop that happened to be mutated instead of copied.

Member

gaearon commented Oct 4, 2017

I think @sophiebits’s example in #3226 (comment) also presents a less obvious place where it could break:

class Parent extends React.Component {
  state = { person: { name: 'Dan' } };

  cachedEl = <Child person={this.state.person} />;

  handleClick = () => {
    this.state.person.name = 'Dominic';
    this.forceUpdate(); // Or even this.setState({})
  }

  render() {
    return (
      <div onClick={this.handleClick}>
        {this.cachedEl}
        <Child person={this.state.person} />
      </div>
    );
  }
}

function Child(props) {
  return <h1>Hi, {props.name}!</h1>;
}

In this example, one Child will get updated but the other one doesn’t, even though Child did “nothing wrong”. It didn’t depend on any external data. It just happened to accept a prop that happened to be mutated instead of copied.

@facebook facebook deleted a comment from ernest-bruce Oct 4, 2017

@msuperina

This comment has been minimized.

Show comment
Hide comment
@msuperina

msuperina Oct 5, 2017

It just happened to accept a prop that happened to be mutated instead of copied.

I don't understand this sentence. Are mutable props supported ? Isn't it bad practice that should be avoided at all cost ?

msuperina commented Oct 5, 2017

It just happened to accept a prop that happened to be mutated instead of copied.

I don't understand this sentence. Are mutable props supported ? Isn't it bad practice that should be avoided at all cost ?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Member

It's not nice but it's supported. I'm pointing out a case that works in React but breaks with this optimization.

Member

gaearon commented Oct 5, 2017

It's not nice but it's supported. I'm pointing out a case that works in React but breaks with this optimization.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Member

Specifically, note that I'm not mutating props but I am mutating some object in the state, and then calling forceUpdate. This has always been supported as it's often the only way to start integrating a legacy codebase with React.

Member

gaearon commented Oct 5, 2017

Specifically, note that I'm not mutating props but I am mutating some object in the state, and then calling forceUpdate. This has always been supported as it's often the only way to start integrating a legacy codebase with React.

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