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

[BUGFIX beta] CPs check presence of getter function #12156

Merged
merged 1 commit into from
Aug 23, 2015

Conversation

cibernox
Copy link
Contributor

Today I spent some time tracking a cryptic stack trace.

It turns out that I typed years: computed('age') instead of years: computed.alias('age')

This will check the presence of the getter function in creation time, instead of failing when you try to access the property, usually 10 minutes after writing it when you have no idea where to look.

@cibernox
Copy link
Contributor Author

Bugfix is probably not the best tag, but I couldn't find a better one.

@stefanpenner
Copy link
Member

is a CP \w setter only considered valid?

@cibernox
Copy link
Contributor Author

I wouldn't consider it valid. Does ES5 to have setters without getter? I think you can have readonly properties, but not writeonly, do you?

@stefanpenner
Copy link
Member

Does ES5 to have setters without getter?

yes

@cibernox
Copy link
Contributor Author

@stefanpenner You can define only a setter, but that's because the getter will have a default (just getting the property). In other words, defineProperty accepts writable flag to make something readonly, but there is no way of making something writeonly.

A computed property needs to execute something.

@stefanpenner
Copy link
Member

A computed property needs to execute something.
Hide all checks

Actually it doesn't, as the return value of the set populates the cache. Subsequent reads read from the cache. Yes this is order specific but it could be used (i beleve)

(I'm not trying to be pedantic, rather enumerating the possible scenarios this assertion without modification will cause grief for)

I suspect we can just assert that either getter or setter exist, and my concerns are covered.

@cibernox
Copy link
Contributor Author

That cache we are thinking we might remove? :trollface:

Seriously speaking, I can change the assertion to check that you either pass a getter or a setter, but think that a CP with only a setter is a case so weird that we should make it less ergonomic than the most common scenario, a brainfart on the line of:

age: computed('years'),
// or
age: computed({
  het() {},
  set() {}
});

@stefanpenner
Copy link
Member

Seriously speaking, I can change the assertion to check that you either pass a getter or a setter, but think that a CP with only a setter is a case so weird that we should make it less ergonomic than the most common scenario, a brainfart on the line of:

yes weird, but it is possible today. So we should work within the constraints our users currently have, not impose additional.

If the assertion checks that at-least the getter or the setter is present, i think that is a win.

@rwjblue
Copy link
Member

rwjblue commented Aug 20, 2015

age: computed({
  het() {},
  set() {}
});

We can also assert if we find any keys other than get and set, which would catch this.

@cibernox cibernox force-pushed the assert_presence_of_getter_in_cps branch from 1d4a701 to 5b0ab81 Compare August 20, 2015 19:15
@stefanpenner
Copy link
Member

We can also assert if we find any keys other than get and set, which would catch this.

good idea

@cibernox cibernox force-pushed the assert_presence_of_getter_in_cps branch from 5b0ab81 to 8371c60 Compare August 20, 2015 21:12
@cibernox
Copy link
Contributor Author

Ok, added assertions for:

  • Computed properties need to receive either a function or a configuration object as last argument. It guards against:
computed('nothing')
computed('array', []);
  • Computed properties must receive at least a getter or a setter.
computed('emptyobj', {  })
  • When it receives an object, only get & set are allowed keys. Guards against:
computed('nothing', {  
  het() {},
  set() {}
})

Any more idiomatic way of checking this last one. Also, is there any reason to use Ember.isArray in the case instead of Array.isArray?

@cibernox cibernox force-pushed the assert_presence_of_getter_in_cps branch 2 times, most recently from 3ab6eb5 to 91232ec Compare August 20, 2015 23:24
@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2015

Seems to die when loaded by Phantom (see Travis failures). Might be the for ... of ...?

@cibernox cibernox force-pushed the assert_presence_of_getter_in_cps branch from 91232ec to 85d744f Compare August 21, 2015 08:04
@cibernox cibernox force-pushed the assert_presence_of_getter_in_cps branch from 85d744f to f50e6c4 Compare August 21, 2015 14:08
rwjblue added a commit that referenced this pull request Aug 23, 2015
[BUGFIX beta] CPs check presence of getter function
@rwjblue rwjblue merged commit b3b6e47 into emberjs:master Aug 23, 2015
@cibernox cibernox deleted the assert_presence_of_getter_in_cps branch August 23, 2015 15:42
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 this pull request may close these issues.

3 participants