Skip to content

Issues with ES6 classes #2575

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

Closed
ceane opened this issue Nov 20, 2014 · 18 comments
Closed

Issues with ES6 classes #2575

ceane opened this issue Nov 20, 2014 · 18 comments

Comments

@ceane
Copy link

ceane commented Nov 20, 2014

I'm having issues with ES6 classes in React. It seems that esprima-harmony differs from how 6to5 is implementing classes, as well as how classes are implemented in beta versions of browsers.

With esprima-harmony I am able to use classes (using React.createClass(MyClass.prototype)), however,

If I use any of these methods:

with 6to5

React.createClass(MyComponent.prototype);

In Chrome 39 and IE11 Windows 10 Preview

React.createClass(MyComponent);

I get this error: Error: Invariant Violation: createClass(...): Class specification must implement arendermethod.. And if I try using React.createFactory in 6to5 I get this error: TypeError: undefined is not an object (evaluating 'nextElement.props').

I can't figure out what would be so different between Object.defineProperty and directly defining prototypes, other than the fact that properties set on the prototype aren't automatically enumerable.

ES6 code

class MyComponent {
  render() {
    return null;
  }
}

esprima-harmony compiles:

function MyComponent() {}
MyComponent.prototype.render = function() {return null;}

6to5 compiles:

var _classProps = function(child, staticProps, instanceProps) {
  if (staticProps) Object.defineProperties(child, staticProps);
  if (instanceProps) Object.defineProperties(child.prototype, instanceProps);
};

var MyComponent = (function() {
  var MyComponent = 
     function MyComponent() {};

  _classProps(MyComponent, null, {
    render: {
      writable: true,

      value: function() { 
        return null;
      }
    }
  });

  return MyComponent;
})();

How should I resolve this issue?

@sebmarkbage
Copy link
Collaborator

This seems to be a bug in 6to5. Methods in ES6 classes should be enumerable. Please report it to the 6to5 project.

@ceane
Copy link
Author

ceane commented Nov 25, 2014

It seems that methods are not enumerable on ES6 classes.

In a class body, a prototype method declaration:

m(args) { body }

is equivalent to:

m: {
   value: function m(args) { body },
   enumerable: false,
   writable: true,
   configurable: true
}

Prototype getters and setters are similar.

@sebmarkbage
Copy link
Collaborator

That document is outdated. AFAIK, the spec draft has changed since then.

On Nov 25, 2014, at 8:28 AM, Ceane Lamerez [email protected] wrote:

It seems that methods are not enumerable on ES6 classes.

In a class body, a prototype method declaration:

m(args) { body }
is equivalent to:

m: {
value: function m(args) { body },
enumerable: false,
writable: true,
configurable: true
}
Prototype getters and setters are similar.


Reply to this email directly or view it on GitHub.

@ceane
Copy link
Author

ceane commented Nov 28, 2014

Yes it would seem so, however on IE 11 preview and Chrome 39, it seems that still class methods are not enumerable.
Test with IE11, Windows 10 Tech preview

var Y = class H {
  render() {
   return null;
  }
};

var G = new Y();

G.propertyIsEnumerable('render');
> false

Happy Thanksgiving, BTW.

@ceane
Copy link
Author

ceane commented Dec 4, 2014

ES6 draft class definition

Also in the latest upcoming ES6 draft (October 2014), it seems that the PropertyDescriptor of methods on a class are as

  1. Let desc be the PropertyDescriptor{[[Value]]: F, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true}.
    ...
  2. For each ClassElement m in order from methods
    a. If IsStatic of m is false, then
    i. Let status be the result of performing PropertyDefinitionEvaluation for m with argument proto.
    b. Else,
    i. Let status be the result of performing PropertyDefinitionEvaluation for m with argument F.

So it seems that class methods are not enumerable with the current ES6 spec.

@sebmarkbage
Copy link
Collaborator

The first one is just the constructor which is not enumerable.

The rest of the elements uses PropertyDefinitionEvaluation which SHOULD make them enumerable.

On Dec 4, 2014, at 2:15 PM, Ceane Lamerez [email protected] wrote:

ES6 draft class definition

Also in the latest upcoming ES6 draft (October 2014), it seems that the PropertyDescriptor of methods on a class are as

Let desc be the PropertyDescriptor{[[Value]]: F, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true}.
...
For each ClassElement m in order from methods
a. If IsStatic of m is false, then
i. Let status be the result of performing PropertyDefinitionEvaluation for m with argument proto.
b. Else,
i. Let status be the result of performing PropertyDefinitionEvaluation for m with argument F.
So it seems that class methods are not enumerable with the current ES6 spec.


Reply to this email directly or view it on GitHub.

@ceane
Copy link
Author

ceane commented Dec 5, 2014

I see this now, number 7.

  1. Let desc be the Property Descriptor{[[Value]]: propValue, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true}

I will report this to 6to5 as well as the browser vendors.

@therajumandapati
Copy link

Should this be re-opened because of the latest announcement given below ?
https://twitter.com/webreflection/status/560929583974088704

Unfortunately/fortunately, 6to5 already made necessary changes to make them not enumerable.

React version being used: 0.12.2.

class _Header {

    renderItems () {
        return (
                <ul>
                    <li><Link className="btn btn-primary" to="newidea">New</Link></li>
                    <li><Link className="btn btn-primary" to="ideas">Ideas</Link></li>
                </ul>
        );
    }

    render () {
        return (
            <div>
                <h1 className="dashboard-header">Idea Store</h1>
                <br />
                {this.renderItems()}
            </div>
        )
    }
};

export default {Header : React.createClass(_Header.prototype)};

The above code generates a prototype object with the above given methods as properties. But, for (var name in spec) { fails as the properties are not enumerable and hence React doesn't find the render method on the prototype.

@therajumandapati
Copy link

@sebmarkbage Any thoughts ?

@ToucheSir
Copy link

@mvpspl619 FWIW you can pass the class option to 6to5's (or Babel's now, I guess) loose mode to generate classes with enumerable members.

@therajumandapati
Copy link

@ToucheSir Thanks.

As that's a temporary way to make React work with ES6, I am curious to find out if anything is being done on this front to make React work with ES6 non-enumerable class methods in the future release ?

@sebmarkbage
Copy link
Collaborator

The problem with enumerability is that it can't be polyfilled in old IE (we still support IE8). Both the detection and definition. If we want to support IE8 we don't want there to be different React behaviors depending on which browser you use, so we can't depend on detecting enumerability. Otherwise someone may accidentally rely on it working, yet in IE8 it will break (and nobody properly tests IE8 these days).

Luckily, we have first class support for ES6 classes in 0.13.

http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html

@therajumandapati
Copy link

@sebmarkbage Thanks for that.

I have dependency problems with react-router and react-async for using [email protected]. Trying to figure out a solution.

@arnihermann
Copy link
Contributor

@sebmarkbage http://facebook.github.io/react/blog/2015/02/24/react-v0.13-rc1.html states that class methods are no longer enumerable by default. This will break a lot of tests (as jest relies on this property) in one of my projects. Are you aware of any workaround?

@sebmarkbage
Copy link
Collaborator

There is a flag to turn it back to the old behavior as a workaround until we can get it fixed in jest.

@zpao
Copy link
Member

zpao commented Mar 10, 2015

In your preprocessor ReactTools.transform(code, {harmony: true, target: 'es3'})

@jakeboone02
Copy link
Contributor

@zpao Thanks for the target: 'es3' tip. That was driving me crazy.

@sebmarkbage Is there an issue to track in the jest project?

@jbe456
Copy link

jbe456 commented May 1, 2015

This issue occurs now with Traceur since this bug fix : google/traceur-compiler#1873

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants