Add deprecation for old usages of cachable and readOnly in CPs by cibernox · Pull Request #9489 · emberjs/ember.js

cibernox · 2014-11-04T14:04:13Z

Passing opts.cacheable to the constructor of ComputedProperty is deprecated
Passing opts.readOnly to the constructor of ComputedProperty is deprecated
Invoking cacheable in a computed property is preprecated.
Invoking readOnly with aany params in a computed property is deprecated.

-- UPDATE --

After reading #9290 and emberjs/rfcs#12, seems that this PR is the first step to make CP read only by default.

Steps done in this PR:

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.

Steps pending:

Modify CPs to work internally with setter and getters. A way of doing that gracefully without people even noticing it could be count the arity of the function passed to Ember.computed. If is arity <=1, create only a setter. If has arity > 1, assign the same function to getter and setter. Passing an object also works. No deprecations yet.
Deprecate passing functions with arity > 1. Recommend user {get, set} instead.
Implement logic in this RFC: Create writable and/or overridable and determina default values as explained in the code example above.
Deprecate all the things!
Ember 2.0. Delete all the things!

This steps don't need to happen in this PR. I fact I would merge things one step at a time.

rwjblue · 2014-11-04T14:07:55Z

Can you add a test that uses expectDeprecation to confirm this works as desired (deprecates when calling cacheable, but not when you don't).

Can you also deprecate passing opts.cacheable in the CP constructor (same area as your prior PR), and add tests to confirm deprecation scenarios?

rwjblue · 2014-11-04T14:08:25Z

packages/ember-metal/lib/computed.js

Needs to be marked as @deprecated also.

cibernox · 2014-11-04T14:08:27Z

I'm on it.

cibernox · 2014-11-04T14:30:37Z

All done.

You mentioned in the former PR that readOnly should be deprecated too. I can deprecate passing opts.readOnly in the constructor, but there is no equivalent of volatile for readOnly. I think I wouldn't deprecate it.

Because you can pass readOnly(false). I can deprecate to pass any value to readOnly though.

cibernox · 2014-11-04T14:34:46Z

Forget my last comment. I misread your comment in the last PR.

rwjblue · 2014-11-04T14:35:07Z

packages/ember-metal/lib/computed.js

s/alwats/always/

cibernox · 2014-11-04T14:49:54Z

Added readOnly deprecation too.

rwjblue · 2014-11-04T15:13:47Z

There are a few failures, when running with this query-string.

cibernox · 2014-11-04T15:32:37Z

Fixed. Now I invoke readOnly instead of passing the option on the constructor. The readOnly method is inherited form the CP prototype, as some other methods.

Should I rebase?

stefanpenner · 2014-11-04T15:44:30Z

@krisselden r?

rwjblue · 2014-11-04T15:45:04Z

@cibernox - Yes, if you could squash and rebase, that would be great.

cibernox · 2014-11-04T22:57:19Z

Rebased. Done here.

rwjblue · 2014-11-05T00:19:15Z

@cibernox - Thank you. I think we are waiting for @krisselden to have a chance to review before merging.

benlesh · 2014-11-08T06:04:18Z

👍

rwjblue · 2014-11-09T17:21:13Z

This seems good to me.

@stefanpenner / @krisselden - Any tweaks or objections?

cibernox · 2014-11-09T17:31:34Z

For the record, I have implemented emberjs/rfcs#11 on top of this.
Only two failing tests involving super. I'll ask for help later in IRC

cibernox · 2014-11-11T00:25:15Z

@krisselden / @stefanpenner Ping here. #9527 is done and based on this one.

cibernox · 2014-11-13T01:10:49Z

Rebased

stefanpenner · 2014-11-13T04:05:03Z

so far looks good.

@krisselden your eyes would be good

cibernox · 2014-11-17T22:29:00Z

Another ping to @krisselden !

stefanpenner · 2014-11-17T22:30:02Z

I have PM'd him aswell.

proof of receipt:

cibernox · 2014-11-17T22:30:53Z

Thanks!

cibernox · 2014-11-21T01:52:23Z

@krisselden

krisselden · 2014-11-24T22:47:54Z

packages/ember-metal/lib/computed.js

.volatile is more like manual, it may prevent the internal CP caching, but it has no effect on downstream caching that happens in the view layer for example. It is generally used for properties that are manually notified and delegated, like array.length. It really should be an extremely rare occurrence in an app.

So what do you suggest then? Not mention volatile at all?

I suspect he would prefer you don't mention it at all.

@cibernox lol, i see we have driven you to insanity with our slowness. Sorry, we will not let this happen again.

* Passing opts.cacheable to the constructor of ComputedProperty is deprecated * Passing opts.readOnly to the constructor of ComputedProperty is deprecated * Invoking `cacheable` in a computed property is preprecated. * Invoking `readOnly` with aany params in a computed property is deprecated. * Make InjectedProperty inherit `readOnly` from CPs prototype

cibernox · 2014-11-26T22:05:28Z

I would say that NOW its ready to merge.

cibernox · 2014-12-01T12:55:34Z

@stefanpenner ping!

Add deprecation for old usages of cachable and readOnly in CPs

stefanpenner · 2014-12-01T23:37:31Z

👍 sorry, i have meant to review this sooner but i have been sick in bed for the last few days.

cibernox · 2014-12-01T23:51:13Z

Don't worry

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from 64de7e4 to c60c5c6 Compare November 4, 2014 14:07

rwjblue reviewed Nov 4, 2014
View reviewed changes

packages/ember-metal/lib/computed.js

Copy link

Member

rwjblue Nov 4, 2014

Choose a reason for hiding this comment

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

Needs to be marked as @deprecated also.

cibernox force-pushed the deprecate_cacheable_method_in_cps branch 4 times, most recently from b08e76d to 9f48f03 Compare November 4, 2014 14:27

rwjblue reviewed Nov 4, 2014
View reviewed changes

packages/ember-metal/lib/computed.js Outdated

Copy link

Member

rwjblue Nov 4, 2014

Choose a reason for hiding this comment

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

s/alwats/always/

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from 9f48f03 to 3815441 Compare November 4, 2014 14:48

cibernox changed the title ~~Deprecate ComputedProperty.cacheable().~~ Add deprecation for old usages of cachable and readOnly in CPs Nov 4, 2014

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from 904dfc1 to c56e38c Compare November 4, 2014 15:47

cibernox mentioned this pull request Nov 7, 2014

default readOnly CP #9290

Closed

4 tasks

stefanpenner self-assigned this Nov 7, 2014

cibernox mentioned this pull request Nov 10, 2014

RFC: Improved CP Syntax #9527

Merged

8 tasks

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from c56e38c to 56f1b57 Compare November 13, 2014 01:10

krisselden reviewed Nov 24, 2014
View reviewed changes

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from 56f1b57 to bb8d206 Compare November 25, 2014 00:06

cibernox force-pushed the deprecate_cacheable_method_in_cps branch from bb8d206 to dd15598 Compare November 25, 2014 00:21

stefanpenner added a commit that referenced this pull request Dec 1, 2014

Merge pull request #9489 from cibernox/deprecate_cacheable_method_in_cps

29bbb59

Add deprecation for old usages of cachable and readOnly in CPs

stefanpenner merged commit 29bbb59 into emberjs:master Dec 1, 2014

cibernox deleted the deprecate_cacheable_method_in_cps branch December 1, 2014 23:50

rwjblue mentioned this pull request Jan 22, 2018

[BUGFIX beta] Fix development build relationship caching. warp-drive-data/warp-drive#5332

Merged

Uh oh!

Conversation

cibernox commented Nov 4, 2014

Uh oh!

rwjblue commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

rwjblue commented Nov 4, 2014

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

stefanpenner commented Nov 4, 2014

Uh oh!

rwjblue commented Nov 4, 2014

Uh oh!

cibernox commented Nov 4, 2014

Uh oh!

rwjblue commented Nov 5, 2014

Uh oh!

benlesh commented Nov 8, 2014

Uh oh!

rwjblue commented Nov 9, 2014

Uh oh!

cibernox commented Nov 9, 2014

Uh oh!

cibernox commented Nov 11, 2014

Uh oh!

cibernox commented Nov 13, 2014

Uh oh!

stefanpenner commented Nov 13, 2014

Uh oh!

cibernox commented Nov 17, 2014

Uh oh!

stefanpenner commented Nov 17, 2014

Uh oh!

cibernox commented Nov 17, 2014

Uh oh!

cibernox commented Nov 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Nov 26, 2014

Uh oh!

cibernox commented Dec 1, 2014

Uh oh!

stefanpenner commented Dec 1, 2014

Uh oh!

cibernox commented Dec 1, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants