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

New Rule: No raw objects or arrays on class prototypes #2

Closed
mitchlloyd opened this issue Mar 1, 2016 · 7 comments · May be fixed by #7
Closed

New Rule: No raw objects or arrays on class prototypes #2

mitchlloyd opened this issue Mar 1, 2016 · 7 comments · May be fixed by #7

Comments

@mitchlloyd
Copy link
Contributor

In general, prevent users from putting objects raw objects and arrays on class prototypes:

Bad

export default Ember.Component.extend({
  things: [],
  stuff: {}
});

Good

export default Ember.Component.extend({
  init() {
    this._super(...arguments);
    this.things = [];
    this.stuff = {};
  }
});

Should we also prevent raw values (non-functions) as well?

Some Emberisms will need to be whitespaced:

  • classNameBindings,
  • classNames,
  • actions
@mitchlloyd
Copy link
Contributor Author

For whitelisting, it is not clear to me if any of these properties can be set inside of init or if they must be set on the prototype:

  • actions: prototype,
  • attributeBindings: prototype,
  • classNameBindings: prototype,
  • classNames: prototype,
  • concatenatedProperties: prototype?
  • instrumentDisplay: init possible
  • mergedProperties: prototype?
  • queryParams: prototype?

Any others that I'm missing @mixonic, @rwjblue?

mitchlloyd added a commit that referenced this issue Mar 2, 2016
mitchlloyd added a commit that referenced this issue Mar 2, 2016
@mixonic
Copy link
Member

mixonic commented Jul 20, 2016

@mitchlloyd that list seems fine. It just raises that most of those things, like positionalParams, should probably be defined on the class and not on the instance 😢.

I believe only mutable primitives should be flagged invalid by this rule. Immutable values (numbers, strings, etc) can sometimes be better set on init for raw performance, however they don't come with the logical footgun a mutable value brings.

@justinaray
Copy link

Recently, we got trolled pretty hard by acceptance tests that weren't cleaned up due to someone putting mutable models with timers, observers, etc. onto the prototype. So, I went looking for this rule.

Would love to reopen the dialog on this rule if y'all think it's viable.

Whitelisting seems like a maintenance headache at best and error prone at worst. But, I'm not sure if there's a smarter way to figure out what's acceptable to be on the prototype.

Happy to help if needed.

@mitchlloyd
Copy link
Contributor Author

@justinaray This repo predates eslint-plugin-ember which is now the best spot for Ember-related ESLint rules. You'll find what you're looking for there!

@rwjblue
Copy link

rwjblue commented Nov 5, 2017

FWIW, I just released eslint-plugin-ember which adds exactly the rule that is proposed here.

This can probably be closed...

@mixonic
Copy link
Member

mixonic commented Nov 5, 2017

Donezo. Thanks @rwjblue!

@mixonic mixonic closed this as completed Nov 5, 2017
@justinaray
Copy link

Thanks @mitchlloyd and @rwjblue!! Much appreciated. Heading over to eslint-plugin-ember.

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

Successfully merging a pull request may close this issue.

4 participants