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

RFC: Improved CP Syntax #9527

Merged
merged 1 commit into from Jan 2, 2015

Conversation

@cibernox
Copy link
Contributor

commented Nov 10, 2014

This PR implements emberjs/rfcs#11

It is based on #9489, which will be merge soon. Also supersedes #9478 (I'll close it in a minute).

There is no feature flags for this because it mostly a refactor that does not change current semantics. It could be done though.

Tasks

Done in #9489

  • Raise deprecation warnings when passing cacheable or readOnly to the constructor. No extrictly necessary, but is not used anywhere in the code since 2012 and simplifies things.
  • Deprecate usage of cacheable() and readOnly(). Are the default.

Done in this PR:

  • Refactor CPs to work internally with setter and getters. Passing a function still works: If the function has arity < 2 its considered a getter. If arity is >=2 is considered both setter and getter.

Steps in futures PRs:

  • Use new cp syntax all iver the place in ember internals.
  • Although passing functions with arity >= 2 is still allowed, is deprecated. Recommend user {get, set} instead.
  • Implement logic in emberjs/rfcs#12
  • Deprecate all the things!
  • Ember 2.0. Delete all the things!
@stefanpenner

This comment has been minimized.

Copy link
Member

commented Nov 10, 2014

Mostly looks fantastic

Afew points:

  • arity checks on functions are actually pretty slow, if they are used they should be cached
  • I wonder if we should just name the getter get and setter set on the descriptor

Will review again when I'm back online later tonight

Great work!

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 10, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2014

Great.

I've cached the arity of the setter function, since is the only one we care about its number of arguments.
I've named _getter and _setter because there is already a _set function in the prototype.

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2014

Ok, it fails because I am raising a deprecations where I use the same function as getter and setter.
I'll track all later today, when I feel better. I have an awful flu right now.

@joostdevries

This comment has been minimized.

Copy link

commented Nov 10, 2014

such yay :-)

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 11, 2014

@cibernox cibernox referenced this pull request Nov 11, 2014
@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2014

27 files later ... green!

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 13, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

Rebased and squashed.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

this looks fantastic, @wycats / @tomdale does this need to be feature flagged?

@krisselden i would love your review

@g13013

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2014

@cibernox good job !!

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 13, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

@stefanpenner / @wycats / @tomdale I really hope not need to flag this.
It's retrocompatible anyway (although I raise an annoying deprecation)

@g13013

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2014

If tests pass, it would be great to include it in the next release, it's not a breaking change, and the deprecation messages will prepare for the 2.0.0 !

@wycats

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

All new features need to be feature flagged. That's how the train cycle works.

The flag is removed by the build tools once a "Go" decision is reached.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

Also, just to be clear, the deprecations from #9489 should land without feature flags.

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

The problem is that many of this changes are in not where the functionality is defined but where it's used (and its used all over the place).

Options:

1 - What I can do is split this PR in 2, one containing the new syntax behind a FFlag and another updating ember internals to use it. The second will be on hold until the functionality passes the go/no-go decision (how many weeks can take that?). Downside is that we will be testing the feature but not using it in ember itself. Also the PR put on hold will need to be rebased often to keep it aligned with small changes that will mess with it.

2 - Flag every place of ember internals where we are using setter CPs to have both versions. Downsides are: A lot of work and a lot of churn in the code.

I think option 1 is still more reasonable.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

@cibernox - Only public API changes need to be flagged. If we use the new syntax internally, but only expose the new API if the flag is "turned on" that should be just fine. We can upgrade everything to the new syntax internally and only expose the new public API when the flag is enabled.

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2014

I'll try to figure out how do that later today. I don't even know if its possible since ember uses the public api internally too.

2014-11-13 17:51 GMT+00:00 Robert Jackson notifications@github.com:

@cibernox https://github.com/cibernox - Only public API changes need to
be flagged. If we use the new syntax internally, but only expose the new
API if the flag is "turned on" that should be just fine. We can upgrade
everything to the new syntax internally and only expose the new public API
when the flag is enabled.


Reply to this email directly or view it on GitHub
#9527 (comment).

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 13, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2014

@rwjblue I've been trying a few approaches to feature flag the changes in public API this and still use internally the getter/setter syntax but none of them seams acceptable. Code would be ugly as hell.

The problem is that we use internally the same public api (computed()) we want to feature flag, so I would need to have export both computed and _newComputed, change every single import { computed } to import { _newComputed as computed }, and I haven't even decided what to do with macros like not and mapBy.

If we really want to feature flag this, we I would go with the idea of expose the new version behind a feature flag but don't use it internally until it receives a "go" decision.

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Nov 14, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2014

Updated. This PR only contains the new syntax behind a feature flat.

I created another branch applying the new syntax all over the place that will stay on hold until this passes the go/no-go decision.

@cibernox cibernox force-pushed the cibernox:refactor_cps branch Dec 1, 2014

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2014

@stefanpenner This is the next natural step after #9489

This adds the new syntax behind a feature flag, but does not refactor the internals, because it would be massively complicated to do it while the new syntax is flagged.

I have another branch with all the CP's in ember using the new syntax, waiting.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Dec 2, 2014

rwjblue added a commit that referenced this pull request Jan 2, 2015

@rwjblue rwjblue merged commit 3a3c0b3 into emberjs:master Jan 2, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
Member

commented Jan 2, 2015

Landed!

@cibernox - Thank you for your hard work on this one!

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2015

Yay!

@cibernox cibernox deleted the cibernox:refactor_cps branch Jan 2, 2015

@gpoitch

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2015

What's a good strategy for addons with CPs that want to support ember < 1.12.0 but avoid deprecation warnings when >= ? I thought the new syntax was backwards compatible.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

Use my ember-new-computed addon. ;)

@mixonic

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

@jamiechong

This comment has been minimized.

Copy link

commented May 15, 2015

Sorry if this question is in the wrong place...
I'm looking into resolving the deprecation warnings in my app. I've been defining pretty much all my computeds like this:

fullName: function() {
    return this.get('firstName') + ' ' + this.get('lastName');
}.property('firstName', 'lastName'),

Many of my computeds don't require a setter (they're read only).

  1. With this new method of defining computeds, do I have to now explicitly use Ember.computed() ? Or is there a way to make it happen while still using the .property() macro?
  2. What if I don't want to define a setter, is that okay?
@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2015

@jamiechong If your a computed property don't have a setter the syntax you're using is perfectly ok 😁 . Only properties with setter have to use the new syntax.

@jamiechong

This comment has been minimized.

Copy link

commented May 15, 2015

@cibernox - Okay - that's good to know. Thanks!
Now that I look closer, it seems like the deprecation warnings I'm getting are coming from Ember Data. But I assume those will get resolved in time.

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2015

Depredations from ember data were fixed a while ago. What version of ED are you using?

@jamiechong

This comment has been minimized.

Copy link

commented May 15, 2015

Aha that was it. Was using 1.0.0-beta.16.1 (bower update doesn't update ember-data automatically it seems). Using 1.0.0-beta.17 and now they've gone away. Thanks - I'm still new to Ember :)

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2015

You're welcome. Remember to check the irc #emberjs channel and the slack community.

@samselikoff

This comment has been minimized.

Copy link
Contributor

commented May 18, 2015

why the need to return newValue from setters? I found this unintuitive

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2015

@samselikoff Because CPs memoize whatever you return from get/set.

@tomdale

This comment has been minimized.

Copy link
Member

commented May 20, 2015

In theory we could memoize the value passed in by default if you don't provide an explicit return value. However, this closes the door on CPs that can ever become undefined after being set(), which is a rare but sometimes necessary case.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented May 20, 2015

In theory we could memoize the value passed in by default if you don't provide an explicit return value. However, this closes the door on CPs that can ever become undefined after being set(), which is a rare but sometimes necessary case.

I think the return value is the least un-expected memoized value.

@tomdale

This comment has been minimized.

Copy link
Member

commented May 20, 2015

I think the return value is the least un-expected memoized value.

Totally agree, I was just trying to enumerate the constraints for @samselikoff.

@samselikoff

This comment has been minimized.

Copy link
Contributor

commented May 20, 2015

I see. I do feel this is an additional conceptual jump for newbies, though. It's exposing internal implementation details to the public API ("why should I care that CPs memoize the return value? I just want to change some other attributes.")

Would it make more sense to require users to .set(undefined) if they wanted the CP to be set to undefined? Seems like it could be a win, freeing up everyone else from having to know this.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented May 20, 2015

given that:

(a.foo = 1) === 1;

I believe that returning the result from the setter makes sense. The memoizing is an merely an enhancement CP's provide.

We should really also make

a.set('foo', 1) ===1

a PR exists to do this.

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 20, 2015

I do feel this is an additional conceptual jump for newbies, though.

IMHO, the whole concept of setter CP's is a non-newbie concept. Getter only CP's (the default) are massively more common, and solve the majority of use cases for non-expert users.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented May 20, 2015

IMHO, the whole concept of setter CP's is a non-newbie concept. Getter only CP's (the default) are massively more common, and solve the majority of use cases for non-expert users.

it's also extremely easy to learn ;)

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 20, 2015

it's always extremely easy to learn ;)

I agree, but it isn't on the list of things needed to know to make an awesome application...

@samselikoff

This comment has been minimized.

Copy link
Contributor

commented May 20, 2015

Sounds good all. Thx for explaining :)

@samtsai

This comment has been minimized.

Copy link

commented May 29, 2015

Definitely makes more sense after reading the discussion. Thanks for posting the question: @samselikoff. I literally ran into this issue yesterday and had a look at ember internals to understand before seeing the rest of this discussion.

Could we update the example given in the blog (http://emberjs.com/blog/2015/05/13/ember-1-12-released.html) and future 1.12 guides to show the explicit 'return' for setters? Or somewhere document this?

@samselikoff

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

yah I agree that guide needs to be updated, I keep looking at it for reference and forgetting about the return.

@ptgamr

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

Guys, I just got a very weird bug because my setter method does not return value.
Any idea why I need to do this? Is it documented anywhere? Thanks!

@cibernox

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

I've answered the same question a three times already, so that means that clearly we need to better document this somewhere, since people finds it confusing.

The idea is that computed properties should be as aligned as possible to regular ES5 getters/setters (and just wrap the getter in a caching layer). ATM, your need to return a value from the setter function in order to be able to cache the value.

I start to think that this should be changed to be less surprising, but I don't think it will be possible/easy until we have decorators. What we can do however is clarify this point in the documentation, although I'm not sure what is the best place to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.