Skip to content
This repository has been archived by the owner on May 26, 2019. It is now read-only.

Encourage decorator-style Ember.computed/Ember.observer #110

Closed

Conversation

stefanpenner
Copy link
Member

lets encourage best-practices.
TL;DR we want to move away from prototype extensions.

So for today, lets tackle two simple ones.

  • .property -> Ember.computed
  • .observes -> Ember.observer

Further discussions about all other PROTOTYPE_EXTENSIONS, are more complicated and as they may result in development effort. They will go through the formal RFC process.

Topics for later discussion:

  • Ember.A
  • observer(..., fn).on

[edit: wording changes, based on feedback]

@stefanpenner stefanpenner changed the title Prototype extensions kill Prototype extensions Mar 27, 2015
@eviltrout
Copy link

Really? Every ember project I've browsed seems to use them for convenience? When did the tide change?

@balinterdi
Copy link
Contributor

@stefanpenner Should prototype extensions be shunned because they introduce a performance penalty (or in plain English: because they are slow)? I guess the same then goes for observes, which are best avoided anyway when possible.

@stefanpenner stefanpenner changed the title kill Prototype extensions lets encourage not using prototype extensions Mar 27, 2015
@stefanpenner
Copy link
Member Author

Really? Every ember project I've browsed seems to use them for convenience? When did the tide change?

this is part of a larger effort for ember to be a better ecosystem citizen. Basically, its extremely dubious to pollute globals as we currently do.

Also, ES7 will give us annotations, which partially unlocks our path to using the new Class syntax provided by ES6. Transitioning from the computed('foo', 'bar', fn) syntax to the proposed annotation syntax is super mechanical.

@stefanpenner Should prototype extensions be avoided because they introduce a performance penalty (or in plain English: because they are slow)? I guess the same then goes for observes, which are best avoided anyway when possible.

nah, no performance difference for these prototype extensions.

@cibernox
Copy link
Contributor

Will we need to use Ember.A() all over the place? The extensions over array are actually the only extensions I find really useful

@tomdale
Copy link
Member

tomdale commented Mar 27, 2015

This is a very big change and I think we need to be a little bit more deliberate in the messaging, if we want to make this recommendation at all.

I personally find code written with prototype extensions easier to write and parse. Also remember that disabling prototype extensions has a significant performance cost due to the need to Ember.A() observed arrays. That said, prototypes can cause compatibility issues and I am always happy to revisit the decision if the ecosystem has sufficiently changed.

Personally, I would prefer that we frame this less as "let's stop using prototype extensions" and more "let's start aligning our syntax with ES7". Ideally, we could migrate folks over to the annotations API directly with Babel, and only have one mass migration instead of two.

@stefanpenner
Copy link
Member Author

Will we need to use Ember.A() all over the place?

for now, some future RFC process will likely dig into that in more detail. So lets not dig into that here.

The above changes, Ember.computed over function(){}.property() are already supported, and have zero additionally side-affects.

@stefanpenner
Copy link
Member Author

I personally find code written with prototype extensions easier to write and parse. Also remember that disabling prototype extensions has a significant performance cost due to the need to Ember.A() observed arrays.

yes, a more detailed RFC can tackle this one. Ember.A() is a special case, that will require more effort and energy. But this puts us on a better path

@tomdale
Copy link
Member

tomdale commented Mar 27, 2015

@stefanpenner Suggesting that everyone A() arrays is a very large change that will trip up new developers and has the potential to cause performance regressions. I am 👎 on this change which seems hasty; I would be interested in a more thorough transition plan that takes these externalities into account.

Ember.A() is a special case, that will require more effort and energy. But this puts us on a better path

I think this incremental step will cause more chaos than whatever benefits it provides, which are IMO pretty marginal.

@stefanpenner
Copy link
Member Author

This is a very big change and I think we need to be a little bit more deliberate in the messaging, if we want to make this recommendation at all.

I argue that this isn't a big change, this is merely encouraging a different syntax that we already support.

@stefanpenner
Copy link
Member Author

@stefanpenner Suggesting that everyone A() arrays is very large change that will trip up new developers and has the potential to cause performance regressions. I am on this change which seems hasty; I would be interested in a more thorough transition plan that takes these externalities into account.

This PR has nothing to do with this, nore does it recommend this. We are merely encouraging using computed and observers over there prototype extension variants

@tomdale
Copy link
Member

tomdale commented Mar 27, 2015

This PR has nothing to do with this. We are merely encouraging using computed and observer over there prototype extension variants

Ah, I see. Like I said above, I would rather just wait until we have annotations in Babel (which seems very close) and have people migrate en masse at once, rather than have an annoying transition period where new developers will find examples with .property(), computed() and the ES7 annotation syntax.

@stefanpenner
Copy link
Member Author

Ah, I see. Like I said above, I would rather just wait until we have annotations in Babel (which seems very close) and have people migrate en masse at once, rather than have an annoying transition period where new developers will find examples with .property(), computed() and the ES7 annotation syntax.

this already exists, ^^ helps move people towards just computed rather then both property and computed

We can add a blurb in the computed property guide, discussing what .property is, and how and why we are moving away from this.

@cibernox
Copy link
Contributor

In fact I already favour Ember.computed/Ember.observer over the prototype extensions, but the observable interface un EmberArray is massively convenient. As Tom said, it might scare newcomers

@stefanpenner
Copy link
Member Author

but the observable interface un EmberArray is massively convenient

this effort does not touch this, some future investigation can dig into those.

@rwjblue
Copy link
Member

rwjblue commented Mar 27, 2015

I personally find code written with prototype extensions easier to write and parse.

@tomdale - I completely disagree. It is much easier to read when you know the context (normal function, computed property, or observer) of a function before you get to the end. With Ember.computed('someProp', function() { }); the reader immediately knows that it is a computed property AND what dependent keys will cause invalidation. Whereas with function() { }.property('someProp') you have to read the entire function (or skip to the end and then start back at the beginning) before you know it is a computed with specific DK's.

@karlfreeman
Copy link

For the computed properties change would it not be best to start off with re-writing the computed properties section first? Potentially providing a clear explanation as to why its a "best-practice" going forward? and a nod towards the current way people are writing it? (e.g removing the secion on alternate invocation and providing a replacement for the prototyped way of doing it)

@stefanpenner
Copy link
Member Author

For the computed properties change would it not be best to start off with re-writing the computed properties section first?

we can do that as part of this PR, and i am very much on-board with fleshing that part out. (Hence submitting the PR for review)

@karlfreeman
Copy link

@stefanpenner Sure, I guess in my eyes it might be a better straw man to knock down than just the syntax update. 👍.

@stefanpenner
Copy link
Member Author

All this input is great, tweet + pr is succeeding in getting eyes and feedback!

@karlfreeman
Copy link

@stefanpenner The more I cruise through the docs, the more I realise theres a general inconsistency with how prototype extensions are mentioned / referenced. Perhaps a PR before this one could be a tightening up on that front. computed properties' "alternative invocation" vs observer's "without prototype extensions". Both of which ideally would link to their respective sections within the disabling prototype extensions page.

@stefanpenner
Copy link
Member Author

Perhaps a PR before this one could be a tightening up on that front. computed properties' "alternative invocation" vs observer's "without prototype extensions".

I'm curious why this that can't be single consolidated effort? That would help ensure the tone/intent is unified on this topic.

@karlfreeman
Copy link

@stefanpenner Its close as it stands right now so I think its a matter of simply moving the two sections in the computed properties and observer pages into one place. Instead of trying to explain them differently in different places?

@stefanpenner
Copy link
Member Author

@stefanpenner Its close as it stands right now so I think its a matter of simply moving the two sections in the computed properties and observer pages into one place. Instead of trying to explain them differently in different places?

that sounds good, I will try to word smith after my next two meetings. If you have any ideas feel free to submit a PR to my branch, or leave words here in comment.

@karlfreeman
Copy link

@stefanpenner Both pages could then signpost towards a single place for a potential "upcoming" syntax change? Or not ;).

@stefanpenner
Copy link
Member Author

Both pages could then signpost towards a single place for an potential "upcoming" syntax change? Or not ;).

both syntaxes are currently available, this merely demonstrates that one that enables ember to be a better citizen in the JS ecosystem.

@ghost
Copy link

ghost commented Mar 27, 2015

👍, the proposed form reads much better.

Also, If we do observer and computed, please also consider including on!

@bcardarella
Copy link

At DY we've worked without the Function.prototype extensions for several projects and personally I find it to be more readable. However, I agree with @tomdale with the issues around Ember.Array. I suspect this will increase the WTFs/minute for people getting into Ember. With the effort around 2.x being to lower the barrier of entry it seems odd to then take a step backwards in this regard.

It seems that most everyone here is in favor of suggesting the Function.prototype extensions not be used. Is there a compromise where we can suggest turning off just the Function.prototype extensions, rather than all or nothing? I realize there is a desire to avoid collisions with other libraries but as a first step this might be the best path.

@stefanpenner
Copy link
Member Author

@abuiles it should likely not use a top level command name like that, maybe something namespaced

@j-carvalho
Copy link

Don't forget about the .on notation as well. Ember.on usage is also required. This plugin still doesn't cover this use case.

Also, there are some places in the docs for observers where .observes('name').on('init') is specified, when we want the observer to be called on the initialization.

How should we handle these cases?

Ember.on('init', Ember.observer('name'... )) or Ember.observer('name, Ember.on('init', ...)) ?

@stefanpenner
Copy link
Member Author

Also, there are some places in the docs for observers where .observes('name').on('init') is specified, when we want the observer to be called on the initialization.

we can/could make Ember.observer(..., fn).on work without prototype extensions. Either by decorating the functions, or doing some adapter style pattern. I personally prefer the adapter style pattern, although it would be more work.

  • But the docs should be updated to meet the current limitations

@MiguelMadero
Copy link

move away from prototype extensions.

Is there an idea of the timeframe for this?

I think it makes sense to update the docs and change defaults so they're off by default would make sense, but completely removing them seems like unnecessary overhead for a lot of existing apps for 2.0. We could probably move that to an Add-on.

clock.set('_seconds', seconds + (1/4));
}
}, 250);
}), 'init')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://emberjs.com/api/classes/Ember.html#method_on
on (eventNames, func)

isn't this backward? Shouldn't it be
Ember.on('init, Ember.observer('_seconds', function(){...}));

@eccegordo
Copy link

@stefanpenner what is the syntax for volatile().

Is it just tacked on at the end still?

This

status: function() {
  // return status
}.property('foo', 'bar').volatile()

becomes this

status: Ember.computed('foo', 'bar', function() {
  // return status
}).volatile()

@stefanpenner
Copy link
Member Author

Is it just tacked on at the end still?

yes

@mixonic
Copy link
Member

mixonic commented Jul 11, 2015

ping would really love to have this merged.

@tomdale
Copy link
Member

tomdale commented Jul 12, 2015

I retract whatever rejections I may have had and am 👍 on migrating towards non-prototype-using examples, with the exception as discussed above about arrays. If ember-watson can do this transition for folks automatically I see no reason to push back.

@bcardarella
Copy link

@tomdale aren't most of the array prototype extensions pulled out anyway now?

@mixonic
Copy link
Member

mixonic commented Jul 12, 2015

@bcardarella like pushObject? That and similar methods should still be present.

@bcardarella
Copy link

@mixonic I'm referring to emberjs/ember.js@62916e8

@ghost
Copy link

ghost commented Jul 12, 2015

http://emberjs.jsbin.com/devesujowo/9/edit?html,js,output
I agree with @tomdale.

I also think the guide needs a section on Arrays, explaining that the vanilla push, shift and pop don't fly with Ember, that they need to use pushObject, shiftObject or popObject instead.

@mixonic
Copy link
Member

mixonic commented Jul 12, 2015

@bcardarella ah, I believe that PR just removes support for ES3 polyfills (IE8). We will continue to extend native array types with pushObject etc for the foreseeable future AFAIK.

@SaladFork
Copy link
Contributor

Is it just tacked on at the end still?

yes

So the proposed looks like:

With Extensions Without Extensions
function () { ... }.property('foo') Ember.computed('foo', function () { ... })
function () { ... }.observes('foo') Ember.observer('foo', function () { ... })
function () { ... }.on('bar') Ember.on('bar', function () { ... })¹
function () { ... }.volatile() Ember.*(function () { ... }.volatile()²
function () { ... }.readOnly() Ember.*(function () { ... }.readOnly()²

¹ Though the discussion suggests that Ember.*(function () { ... })on('bar')² might also be maintained
² Where * is either computed or observer

It seems odd that we're moving from a consistent interface to one that can take one of two forms.

Of course, this mostly becomes a non-issue with ES7 decorators where @computed and @readOnly are similar.

@pixelhandler
Copy link
Contributor

One valuable note regarding the convention to move away from reliance on prototype extensions in the default documentation is to promote addon development. Addon development requires development to support no prototype extensions by default. In addition to the default documentation supporting the Ember.computed syntax over .property() it will be important do document when it's beneficial to use prototype extension e.g. with Em.A(). I for one really like a common way to write ember code in apps and addons :)

@stefanpenner
Copy link
Member Author

it will be important do document when it's beneficial to use prototype extension e.g. with Em.A().

will shortly go away entirely.

One valuable note regarding the convention to move away from reliance on prototype extensions in the default documentation is to promote addon development.

yes

@michaelrkn
Copy link
Contributor

This PR has led to some great discussion, as well as some other PRs that have moved the state of the docs forward. However, the PR itself is out of date and mostly changes files that don't exist any more. I've just opened up #455 to summarize what needs to be done so that somebody can pick up the remaining work. If a couple of you who have been involved in this discussion can make sure that the issue correctly describes what needs to be done, that would be great! Thanks a bunch!

@michaelrkn michaelrkn closed this Jul 22, 2015
abought added a commit to abought/experimenter that referenced this pull request Mar 15, 2016
Also, change code to emphasize ember.computed over function prototype extensions (per ember evolving recommendations)

http://blog.ksol.fr/ember-js-declaring-computed-property-and-observers/
emberjs/guides#110

[#LEI-157]
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.