Skip to content
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

ES6 classes broken in various ways #123

Closed
buschtoens opened this issue Jul 12, 2017 · 10 comments
Closed

ES6 classes broken in various ways #123

buschtoens opened this issue Jul 12, 2017 · 10 comments

Comments

@buschtoens
Copy link
Collaborator

buschtoens commented Jul 12, 2017

Live Demo: https://buschtoens.github.io/ember-decorators-es6-classes/
Source Code: https://github.com/buschtoens/ember-decorators-es6-classes

In the demo linked above I compare two equivalent components. The only difference is that one was defined using the traditional Ember.Object model and the other using native ES6 classes.

// classic-component.js
export default Component.extend({
  params: null,

  foo: 'bar',

  setOnInit: null,

  setOnDidReceiveAttrs: null,

  @computed('foo', 'setOnInit', 'setOnDidReceiveAttrs')
  computed(foo, setOnInit, setOnDidReceiveAttrs) {
    return `${foo} - ${setOnInit} - ${setOnDidReceiveAttrs}`;
  },

  @on('init')
  eventedInit() {
    set(this, 'setOnInit', Date.now());
  },

  @on('didReceiveAttrs')
  eventedDidReceiveAttrs() {
    set(this, 'setOnDidReceiveAttrs', Date.now());
  }
}).reopenClass({
  positionalParams: 'params'
});
// es6-component.js
export default class extends Component{
  static positionalParams = 'params';

  params = null;

  foo = 'bar';

  setOnInit = null;

  setOnDidReceiveAttrs = null;

  @computed('foo', 'setOnInit')
  computed(foo, setOnInit) {
    return `${foo} - ${setOnInit}`;
  }

  @on('init')
  eventedInit() {
    set(this, 'setOnInit', Date.now());
  }

  @on('didReceiveAttrs')
  eventedDidReceiveAttrs() {
    set(this, 'setOnDidReceiveAttrs', Date.now());
  }
}

Diff between both transpiled files: https://gist.github.com/buschtoens/29fbaec09b4d9c94443d3c175f4701ad/revisions?diff=split

This might only be tangentially related to this project, but since the README features ES6 classes so prominently, I feel like this is the right place to report this.

Multiple issues arise when using ES6 class syntax:

  1. Initially provided attributes do not overwrite default property values. Only after triggering didReceiveAttrs by subsequently updating an attribute the default value is replaced.

  2. Events don't work.

I'm sure there's more, but these are the most obvious. I also had some problems with class properties being ignored altogether, but I can't seem to reproduce it in a slimmed down example.

@buschtoens
Copy link
Collaborator Author

The first problem is explained by the fact that constructor is called after init and didReceiveAttrs.

@buschtoens
Copy link
Collaborator Author

buschtoens commented Jul 12, 2017

Correction: constructor is called before init, however the constructor of Ember.CoreObject instantly calls init etc, before returning to the derived constructor.

export default class extends Component {
  foo = 'bar';

  constructor() {
    console.log('constructor (pre super)');
    super();
    console.log('constructor (post super)', this.foo);
  }

  init() {
    super.init(...arguments);
    console.log('init', this.foo);
  }

  didReceiveAttrs()  {
    this._super(...arguments);
    console.log('didReceiveAttrs', this.foo);
  }
}
constructor (pre super)
init qux
didReceiveAttrs qux
constructor (post super) bar

The transformed Babel output makes clear why foo has a different value:

    function _default() {
      _classCallCheck(this, _default);

      // code before `super();`
      console.log('constructor (pre super)');

      // base constructor gets called here
      var _this = _possibleConstructorReturn(this, (_default.__proto__ || Object.getPrototypeOf(_default)).call(this));

      // default values are set
      _this.foo = 'bar';

      // code after `super();`
      console.log('constructor (post super)', _this.foo);
      return _this;
    }

So that's an explanation for the first problem. Second problem still unsolved.

Maybe this issue should be moved to Ember.js itself?

@pzuraq
Copy link
Contributor

pzuraq commented Jul 12, 2017

We've been discussing and putting together an RFC for the behavior of Ember.Object's constructor, which was not public API up until this point. So far it looks like it does need to be refactored, but class properties still won't be overwritten by values passed into create. Instead, you'll have to intercept them in the constructor:

export default class extends Component {
  constructor(props) {
    props.foo = props.foo === undefined ? 'bar' : foo;

    super(props);
  }
}

This is a little inconvenient and counterintuitive compared to today's behavior, but it makes much more sense if/when components start behaving more like Glimmer components where all args are passed in to the component on args:

export default class extends Component {
  foo = 'bar'

  constructor(props) {
    super(props);

    console.log(this.foo); // 'bar';
    console.log(this.args.foo); // 'qux';
  }
}

This is what sold me on keeping the behavior this way, it actually gets rid of the merge behavior of old components and creates a clear separation between args that are passed in and properties that are defined on the class, one that cannot be overwritten. However, I do think the new behavior will be unintuitive to current Ember users.

@dfreeman
Copy link
Contributor

@pzuraq isn't it a hard error to access this in a constructor before calling super()?

@buschtoens
Copy link
Collaborator Author

@dfreeman yup, but these two lines can be swapped, I think

@pzuraq
Copy link
Contributor

pzuraq commented Jul 13, 2017

That's a typo, will update in a sec. The idea is that you modify the properties as the come in, before they're set on the object.

@dfreeman
Copy link
Contributor

👍 Gotcha, just making sure I understood correctly

@buschtoens
Copy link
Collaborator Author

Just noting this down for fellow sufferers. There are a few class attributes like tagName or classNameBindings that need to be specified before the base class's init() method is called.

There are two ways that I know of. I currently prefer the first one because of the slightly less verbose syntax:

export default class extends Component.extend({
  styles,
  tagName: 'span',
  localClassNameBindings: ['iconTrueColored', 'iconFalseColored']
}) {
 layout = layout;

 // ...
}
export default class extends Component {
  layout = layout;

  init() {
    // set class attributes first
    this.tagName = 'span';
    this.styles = styles;
    this.localClassNameBindings = ['iconTrueColored', 'iconFalseColored'];

    // then call base class's init
    super.init(...arguments);

    // other init stuff goes here
  }

  // ...
}

@rwjblue
Copy link
Contributor

rwjblue commented Jul 19, 2017

We should add some decorators specifically for handling those special component API's (tagName, classNames, classNameBindings, are there others?). Can you open a separate issue for that?

@pzuraq
Copy link
Contributor

pzuraq commented Jul 31, 2017

Most of these issue have been addressed in the ES Classes RFC, going to close this in favor of discussion there and fixes upstream in CoreObject

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

4 participants