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

Mocha examples should not use arrow functions #318

Closed
awwx opened this issue Mar 31, 2016 · 9 comments

Comments

@awwx
Copy link

commented Mar 31, 2016

The Meteor Guide Mocha examples shouldn't use arrow functions, says the Mocha documentation:

Passing arrow functions to Mocha is discouraged. Their lexical binding of the this value makes them unable to access the Mocha context, and statements like this.timeout(1000); will not work inside an arrow function.

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

Oh interesting, I didn't know mocha used this. Sounds like a pretty legitimate thing to change, and we should probably do it in the Todos app as well: https://github.com/meteor/todos

Kind of a bummer, since it makes all the tests a lot more verbose, but it is the way it is!

@tmeasday something to remember.

Would you mind submitting a PR?

@awwx

This comment has been minimized.

Copy link
Author

commented Mar 31, 2016

Yup, I ran into this today when I needed to call this.slow()

Would you mind submitting a PR?

Sadly I don't have enough time in my schedule to submit a PR for the Meteor Guide.

You might like to consider adding a "pull-requests-wanted" label to the issue, and then in the README encourage people who'd like to contribute to check those issues. (See for example https://github.com/meteor/meteor/labels/pull-requests-encouraged ... notice how you can give someone a URL which displays all the issues with that label).

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

Great idea! The guide is a great first place for people to contribute, we should definitely encourage that.

@hwillson

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Hi guys - I can submit a quick PR for this. Would you prefer to see anonymous functions or named functions used in the Mocha examples? In either case we'll need to adjust the eslint rules a bit - for example if you want anonymous:

/* eslint-env mocha */
/* eslint-disable func-names, prefer-arrow-callback */

describe('something', function () {
  it('something', function () {
    // blah
  });
});

or if you want named:

/* eslint-env mocha */
/* eslint-disable prefer-arrow-callback */

describe('something', function describe() {
  it('something', function it() {
    // blah
  });
});

Let me know - thanks!

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

Damn that's a bit unfortunate. I don't have a preference personally though.

@awwx

This comment has been minimized.

Copy link
Author

commented Mar 31, 2016

@hwillson given that the tests are already described with the first argument to describe and it, it would be needlessly verbose to name the function. (In support note how in the Mocha docs all the functions are anonymous).

As an aside, it's common to need to use function instead of =>. For example, the function you pass to Meteor.publish is called with this set to the publish handler object, which wouldn't work if you used an arrow function. A lint rule that said "always use => in preference to function" wouldn't work for a lot of things.

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

As a personal note, I'd be pretty happy if this simply stopped existing.

@hwillson

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

@awwx Yes, I agree with using anonymous functions in this case (I just wanted to check before submitting the PR). With regards to having a lint rule that recommends using arrow functions (where relevant), that's what the Airbnb rules do (that MDG is using). So to use anonymous functions with tests we'll need to include:

/* eslint-disable func-names, prefer-arrow-callback */

Not a huge deal, just something extra to consider.

PR coming shortly. I'll aim to fire a similar PR in for the todos app as well.

Just to add - there is some discussion about leveraging arrow functions in Mocha 3.

@stubailo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2016

Thanks for the PR @hwillson! Going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.