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

Make sure ActionCreators will work with es6 arrow syntax. Fixes #83 #84

Merged
merged 6 commits into from
Jan 26, 2015
Merged

Make sure ActionCreators will work with es6 arrow syntax. Fixes #83 #84

merged 6 commits into from
Jan 26, 2015

Conversation

therealcisse
Copy link
Contributor

💄

In some es6 transpilers, ES6 arrow functions are bound to the current scope making it impossible to call this.dispatch(...) or other methods of the ActionCreators inside the callback.

By passing context as the last argument to the ActionCreator, one can use context.dispatch(...) or other methods of the ActionCreator.

Since context is passed as the last argument, all existing code work as is and if you don't use es6 arrow syntax, you're not affected.

💄

ES6 arrow functions are bound to the current scope making it impossible to call `this.dispatch(...)` or other methods of the ActionCreators inside the callback. 

By passing context, one can use `context.dispatch(...)` or other methods of the ActionCreator.

Since context is passed as the last argument, all existing code work as is and if you don't use es6 arrow syntax, you're not affected.
@therealcisse
Copy link
Contributor Author

Fixes #83

@jhollingworth
Copy link
Contributor

Damn, I knew messing with the function context would come back to bite me!

Arrow operators are technically not necessary however I understand that people are going to use them for syntactic sugar. The only function that is actually unique is dispatch so can we just pass that in instead?

var UserActionCreators = Marty.createActionCreators({
  addUser: UserActions.ADD_USER((name, email, dispatch) => {
    dispatch(name, email);
  })
});

@therealcisse
Copy link
Contributor Author

The only function that is actually unique is dispatch:

Nice catch. Passing dispatch will be the best idea.

And also, I think not extending actionContext with creator like this:

        function actionContext() {
          return {
            dispatch: function () {
              return dispatch({
                type: actionType,
                handlers: handlers,
                arguments: arguments
              }, properties);
            }
          };
        }

That will pass all tests, but am not so sure it won't break some code out there. Suggestions?

@jhollingworth
Copy link
Contributor

Good stuff.

creator needs to be mixed in so you can call other functions in the action creator

var UserActionCreators = Marty.createActionCreators({
  addUser: UserActions.ADD_USER((name, email, dispatch) => {
    dispatch(name, email);
    this.somethingElse();
  }),
  somethingElse: UserActions.SOMETHING_ELSE()
});

@therealcisse
Copy link
Contributor Author

I'm using webpack with es6-loader and the above code will be transpiled to:

    var UserActionCreators = Marty.createActionCreators({
      addUser: UserActions.ADD_USER(function(name, email, dispatch)  {
        dispatch(name, email);
        this.somethingElse(); // Fails since this != UserActionCreators here, use UserActionCreators.somethingElse() instead
      }.bind(this)),
      somethingElse: UserActions.SOMETHING_ELSE()
    });

But from an es5 point of view, it makes perfect sense.

I will update the docs for es6 users to explicitly use the global ActionCreator singleton in their callbacks.

Thanks.

@jhollingworth
Copy link
Contributor

This does feel a bit like a misuse of the syntax. Fat arrow function's are designed to ensure that the function context is set to the current context. It's terseness is just a side effect.

I really don't see what value fat arrow functions bring here except removing 6 characters. I'm wondering if its just better to advise against using them here?

@therealcisse
Copy link
Contributor Author

I'm wondering if its just better to advise against using them here?

That works for me. I updated the docs.

jhollingworth added a commit that referenced this pull request Jan 26, 2015
Make sure ActionCreators will work with es6 arrow syntax. Fixes #83
@jhollingworth jhollingworth merged commit 28e0ff1 into martyjs:master Jan 26, 2015
@jhollingworth
Copy link
Contributor

👍 thanks :)

@therealcisse therealcisse deleted the actionCreators_es6 branch January 27, 2015 17:07
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.

2 participants