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

Idea: bind actions automatically in MobX 3 #699

Closed
mweststrate opened this issue Dec 7, 2016 · 7 comments
Closed

Idea: bind actions automatically in MobX 3 #699

mweststrate opened this issue Dec 7, 2016 · 7 comments

Comments

@mweststrate
Copy link
Member

mweststrate commented Dec 7, 2016

action could automatically apply "autobinding" in many cases, similarly how @autobind or React.createClass do this. I think this could help beginning programmers to make less mistakes.

Disadvantage is that this is more memory consuming when using classes / prototypes. Class methods are normally stored only on the prototype, but need to be stored on instances to achieve binding.

// Class syntax
class A {
	@action act = () => {
		// bounded in MobX 3
	}

	@action act() {
		// unbounded in MobX 2. Bind in MobX 3?
	}
}

// Plain object syntax
const a = observable({
	act: action(() => {
		// unbounded, cannot be bound
	}),
	act: action(function() {
		// unbounded. Bind in MobX 3?
	})
})

// constructor function
function A() {
	extendObservable(this, {
		act: action(() => {
			// bound by 'coincidence'; arrow binds already to correct this in MobX 2
		}),
		act: action(function() {
			// unbounded in MobX 2, bind in MobX 3?
		})
	})
}
@andykog
Copy link
Member

andykog commented Dec 7, 2016

-1, binding methods to instance is completely unrelated to action purpose.

@mattruby
Copy link
Member

mattruby commented Dec 7, 2016 via email

@jamiewinder
Copy link
Member

There was similar discussion in the React team as I recall (about how createClass auto bound, but Component didn't). The reason they went against it is basically the reason @andykog stated; it would amount to extra magic which, though well intentioned would only really confuse beginners in the long run as it isn't something other parts of ES6 do, though you may expect it to.

@urugator
Copy link
Collaborator

urugator commented Dec 8, 2016

I don't use decorators, so my constructors are cluttered with:
this.method = Mobx.action(this.method.bind(this));
It's annoying, but I agree with others that autobinding shoudn't be done implicitly and in my case it woudn't help much anyway.
You could introduce seperate @boundAction, but It's probably not neccessary - user needs to deal with autobinding on non-mobx-actions as well, so he probably already uses some other means like mentioned @autobind.

@mweststrate
Copy link
Member Author

I like the idea of boundAction, quite clear, not too verbose. Needs some explanation to beginning JS devs what the difference between boundAction and action is, but I think that is your fate when learning the language :-P

@JabX
Copy link

JabX commented Dec 10, 2016

I'm using somewhere in my code the fact that JS methods (and actions too) aren't bound to copy an object method from an object to an another and have the method work on the object it is attached too.

Something like:

const obj1 = observable({prop: 1});
obj1.set = action(function() { this.prop = 2 });

const obj2 = observable(toJS(obj1));

// Now I have the expected behaviour of :
obj1.set() // ==> obj1.prop = 2
obj2.set() // ==> obj2.prop = 2

This is how Javascript works and action shouldn't tamper with it. I agree that in almost all cases autobinding this is a good idea, but sometimes it isn't.

mweststrate added a commit that referenced this issue Dec 25, 2016
@mweststrate
Copy link
Member Author

Closing this issue as [email protected] 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!

Use action.bound to create bound actions

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

6 participants