ES7 property initializers disappear when combined with certain decorator functions

[namuol]

(Note: I encountered this while using react-transform-webpack-hmr, but I believe the issue to be internal to createProxy.)

When using a @decorator function that copies the original component-class into a new one, static properties specified using ES7 initializers are unaccounted for.

For example, here's a slightly modified version of App.js from react-transform-boilerplate:

import React, { Component } from 'react';

// This is TypeScript's implementation of Extend:
var __extends = function (d, b) {
  for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
  function __() { this.constructor = d; }
  __.prototype = b.prototype;
  d.prototype = new __();
};

function decorate (BaseComponent) {
  function DecoratedComponent (...args) {
    console.log("Running decorated constructor!");
    BaseComponent.apply(this, ...args)
  }

  __extends(DecoratedComponent, BaseComponent);

  return DecoratedComponent;
}

@decorate
class Number extends Component {
  static defaultProps = {
    value: 1,
  };

  render() {
    return <h1>{this.props.value} (defaultProps={'' + Number.defaultProps})</h1>;
  }
}

export class App extends Component {
  render() {
    return (
      <div>
        <Number />
        <Number value={42} />
      </div>
    );
  }
}

We expect the rendered output to look like this:

But instead we see this:

For some reason, Counter.defaultProps is not transferred in TypeScript's implementation of __extends when it is defined with ES7 generators.

It may have something to do with how __extends uses hasOwnProperty, but I'm not familiar enough with how react-proxy works under the hood to really take a stab at what might be going on, here.

• If I remove the @decorator, it works as expected.
• If I change @decorator to use ES6's extends, (i.e. class DerivedComponent extends Component ...), it works as expected.
• If I specify defaultProps after the decorator runs (i.e. Counter.defaultProps = ...), it works as expected.

I first encountered this problem when using react-free-style, which is written in TypeScript and provides a decorator function that extends components.

The natural workaround is to avoid static ES7 property initializers, but many of us are already using them, as encouraged by React.

I forked react-transform-boilerplate to make it easier to reproduce the problem:

git clone https://github.com/namuol/react-transform-boilerplate-es7-initializer-bug
cd react-transform-boilerplate-es7-initializer-bug
npm install
npm start
open http://localhost:3000

[gaearon]

Can you please add a failing test to react-proxy?
You can use existing tests as template.

[namuol]

No problem -- I'll try to get something written this weekend.

[namuol]

I couldn't reproduce the problem in isolation -- it occurs when react-proxy is used along with babel-plugin-react-transform -- but I think I've got a handle on what's going on here now.

---

In a nutshell:

Decorators execute in the order opposite that they appear, and react-transform pushes its decorators at the end of the decorators list, so they therefore execute before any pre-existing decorators.

This means that the proxied version of the class is actually what's passed to @decorate in the example code, above.

Since react-proxy uses __proto__ to copy static methods and properties, the b.hasOwnProperty(p) of TypeScript's __extend method fails, and none of the static properties/methods are copied over.

---

There are at least two potential fixes:

1. In react-proxy: Manually copy the static methods and properties directly onto the proxied class
2. In babel-plugin-react-transform: Insert the plugin's decorator at the front of the decorators list so that it executes after all pre-existing decorators

Either one of these will fix this particular issue, but I get the feeling both fixes should be applied; what do you think?

In the meantime, I'll work on a PR to test that hasOwnProperty behaves as expected on proxies.

[gaearon]

Manually copy the static methods and properties directly onto the proxied class

Can you try to do this (without breaking the tests)? Another option is to put getters on all static properties/methods and proxy them to the newest available version.

Insert the plugin's decorator at the front of the decorators list so that it executes after all pre-existing decorators

That won't work. We want to wrap the original class because in 90% of use cases decorator is higher-order component—not a monkeypatching pseudo-inheritance thing like __extend.

[namuol]

Can you try to do this (without breaking the tests)? Another option is to put getters on all static properties/methods and proxy them to the newest available version.

Here's what I have so far.

I added some simple tests that compare the results of getOwnPropertyNames on the proxy and the original component class. (Edit: In hindsight, these are the wrong tests, but these changes do fix the issue in the react-transform-boilerplate fork I made to reproduce the issue. I'll write a proper test before submitting a PR.)

This implementation almost passes every test; I just need to fix this last one.

I'll try to find a complete solution within the next few days; in the meantime, let me know if you think this is the right direction. Thanks!

[gaearon]

Direction seems right. 👍
Thank you!

[gaearon]

@namuol

Can you please see if #29 fixes the issue for you? I was writing it offline so unfortunately I didn't use your code, but it seems to pass the test cases (and I added a bunch of new ones too).

[gaearon]

#29 seems to have fixed the issue in your project.

[namuol]

No worries about not using the changes I had -- my solution was incomplete
anyway. I'm busy right now but I'll make sure to test this out later
tonight; if the tests pass I'm pretty confident it should fix #23. Thanks!

Update: Upgrading to [email protected] fixes the issue as expected. 👍

On Fri, Sep 25, 2015 at 3:06 AM, Dan Abramov [email protected]
wrote:

@namuol https://github.com/namuol

Can you please see if #29 #29
fixes the issue for you? I was writing it offline so unfortunately I didn't
use your code, but it seems to pass the test cases (and I added a bunch of
new ones too).

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

[gaearon]

Great. Should be fixed in [email protected] and [email protected].
Thank you very much for reporting and taking the initial stab at it.