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

Remove inferring argumentless functions to computed properties in observable #532

Closed
mweststrate opened this issue Sep 1, 2016 · 14 comments
Assignees

Comments

@mweststrate
Copy link
Member

See #421. Breaking change so for next major

For migration purposes MobX should warn in a next minor if an function is passed without marking it either with asReference, computed or get

@mattruby
Copy link
Member

mattruby commented Sep 2, 2016

So this change will mark the beginning of 3.0.0?

On Sep 1, 2016 4:21 PM, "Michel Weststrate" notifications@github.com
wrote:

See #421 #421. Breaking change so
for next major

For migration purposes MobX should warn in a next minor if an function is
passed without marking it either with asReference, computed or get


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#532, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIrcmwntQwtWGcj6D5MTA9CHOpAvcsYks5ql0HBgaJpZM4JzJOY
.

@hccampos
Copy link

hccampos commented Sep 2, 2016

Sounds like a welcome change. In my case it shouldn't even break anything. I usually only pass pure data into observable and extendObservable and let the decorators take care of computed, observable and action.

@andykog
Copy link
Member

andykog commented Sep 8, 2016

@mweststrate, just to make it clear, we replacing syntax like this:

observable({
   x: () => this.y
});

with equal alternative:

observable({
   x: computed(() => this.y)
});

right?

@mweststrate
Copy link
Member Author

Both are still supported, first will be deprecated by this story, idiomatic will be:

observable({
   get x() { return this.y }
});

Biggest advantage is that it avoids subtle errors with this (your example for example would have an undefined this)

@andykog
Copy link
Member

andykog commented Sep 8, 2016

@mweststrate, basically, ability to define getter in current context may be very useful, and thats how it worked before, so I wouldn't undervalue it, especially in terms of 3.0.0 migration

@mweststrate
Copy link
Member Author

Ha yeah makes me wonder, is get x () => this.y valid syntax? But anyway computed should still be supported as it is concinsistent with the modifiers approach.

@andykog
Copy link
Member

andykog commented Sep 8, 2016

Ha yeah makes me wonder, is get x () => this.y valid syntax?

I think, no :-) What makes you assume this?

@mweststrate
Copy link
Member Author

This change can be tested using mobx@2.7.0-beta

@mweststrate
Copy link
Member Author

Closing this issue as mobx@3.0.0-rc.1 is now available, which addresses this issue. Changelog: https://github.com/mobxjs/mobx/blob/mobx3/CHANGELOG.md#300. The changes in more details: https://github.com/mobxjs/mobx/pull/725/files. Feel free to re-open if questions arise!

@HaveF
Copy link

HaveF commented Dec 31, 2016

@mweststrate It is very pleasure to hear that mobx 3.0 will be out. I was taken to this thread by use computed in 2.7.0's warning message:

[mobx] Deprecated:
In MobX 2.* passing a function without arguments to (extend)observable will automatically be inferred to be a computed value.
This behavior is ambiguous and will change in MobX 3 to create just an observable reference to the value passed in.
To disambiguate, please pass the function wrapped with a modifier: use 'computed(fn)' (for current behavior; automatic conversion), or 'asReference(fn)' (future behavior, just store reference) or 'action(fn)'.
Note that the idiomatic way to write computed properties is 'observable({ get propertyName() { ... }})'.
For more details, see #532: User@1.data.stopPolling

I just try to install 3.0.0-rc.1, but find it is a bit hard to understand how to upgrade the exist code base to this version. Maybe I should wait?

I also find the change log of 2.7.0, but still confused how to use with decorator(could you explain how to upgrade the example below? Example from the official doc):

class Timer {
    @observable start = Date.now();
    @observable current = Date.now();

    @computed get elapsedTime() {
        return (this.current - this.start) + "seconds"
    }

    @action tick() {
        this.current = Date.now()
    }
}

maybe create the new series of egghead videos will be great 😁

@indivisable
Copy link

indivisable commented Feb 1, 2017

Anyone help upgrade this code?

...
@observable panResponder = null;

constructor() {
	this.initPanResponder();
}

@action initPanResponder() {
this.panResponder = PanResponder.create({
...

I tried this but then it does not get called correctly:

this.panResponder = computed(()=> PanResponder.create({

@mweststrate
Copy link
Member Author

@HaveF @indivisable be aware of commenting on closed issues, comments are easily missed :)

@HaveF your example is valid in both MobX 2 & 3

@indivisable
Your question lacks some information to be clear, but I think @observable.ref panResponder = null; is what you need; panResponder is a build in RN thing afaik, so probably you don't want to convert that into an observable itself

@indivisable
Copy link

@mweststrate Thanks! By simply removing the observable seemed to do the trick.

//@observable panResponder = null;

@crossingAreed
Copy link

thanks @mweststrate

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

No branches or pull requests

7 participants