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

skip properties which are not writable or not configurable #111

Closed
wants to merge 2 commits into from

Conversation

capaj
Copy link
Member

@capaj capaj commented Feb 9, 2016

it doesn't make sense to observe those properties,
sorry about all the whitespace changes, tried to get rid of them but my atom always makes them on save.
fixes #110

@mweststrate
Copy link
Member

Much appreciated! Maybe it could even throw if the decorator is added to a
non-configurable property?

On Tue, Feb 9, 2016 at 12:32 PM, Jiri Spac [email protected] wrote:

just realised-we still need a mobservable getter on a non-writable
property. Skipping is only allright for configurable: false. I will try
to get a proper fix fleshed out in the evening.


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

@capaj
Copy link
Member Author

capaj commented Feb 9, 2016

@mweststrate haven't thought about that one. I mostly use it on objects. But yeah, in case you use it on single property, it makes total sense to fail fast and throw.

Are single properties also made reactive via https://github.com/mweststrate/mobservable/blob/master/src/observableobject.ts#L49 ?

@mweststrate
Copy link
Member

Finished this on the feature/2 branch: when using extendObservable or @observable on a non-configurable / non-writable property it will throw. But when using observable(object), it will skip non-configurable / non-writable properties.

@mweststrate
Copy link
Member

Apologies btw, somehow your commits are not visible in the history, I guess I merged in an incorrect manner.

@capaj
Copy link
Member Author

capaj commented Feb 21, 2016

@mweststrate awesome! so this will get released with mobservable 2.0.0 right?

@mweststrate
Copy link
Member

Indeed :)

Op zo 21 feb. 2016 om 23:43 schreef Jiri Spac [email protected]:

@mweststrate https://github.com/mweststrate awesome! so this will get
released with mobservable 2.0.0 right?


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

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.

ES5 property descriptor support is somewhat lacking
2 participants