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

Clobbering this from options might be murky #26

Open
joliss opened this issue Mar 27, 2015 · 1 comment
Open

Clobbering this from options might be murky #26

joliss opened this issue Mar 27, 2015 · 1 comment

Comments

@joliss
Copy link

joliss commented Mar 27, 2015

If I understand the constructor correctly, we're providing the ability to assign arbitrary properties on this through an options hash as a common base class constructor.

I used to do this in early versions of Broccoli plugin classes because it seemed convenient. I stopped doing it when I started running into cases where some library upgrade would cause property names to start colliding with option names. I simply do this.options = options in constructors ever since, and have been much happier with that.

My suggestion would be to make the base class constructor empty, and let people do their own option handling. I realize this might be a breaking change, but perhaps it's worth considering for the next major version?

@stefanpenner
Copy link
Contributor

If I understand the constructor correctly, we're providing the ability to assign arbitrary properties on this through an options hash as a common base class constructor.

You are understanding it correctly.

If an arbitrary foreign object enters, one could do the following without lack of collision.

var a = new Foo({
  options: foreignObject
});

That being said, I personally do prefer the explicit nature of:

https://github.com/ember-cli/core-object/blob/master/benchmarks/create/core.js#L6-L17

Also, as ES6 class super is non-optional, there would be no opt-out for sub-classes if they did not want the auto-assign ability. Without calling super with an empty pojo, which is janky.

One path to consider would just to provide CoreObject.assign which can optionally be included by a user.

User = CoreObject.extend({
  init(options) {
    this._super(options);
    CoreObject.assign(this, options);
  }
});

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

2 participants