Catch arrow functions for modules

[bmish]

Should the qunit/no-arrow-tests rule also catch modules with arrow functions? Or should we have a new separate rule no-arrow-modules?

Bad:

module('my module', (hooks) => {
  // ...
});

Good:

module('my module', function(hooks) {
  // ...
});

[platinumazure]

I'm inclined to think we should enforce this if we know that there are undesirable side effects from doing this in modules. @bmish Can you speak to that?

Even if there are no actual problems, I could understand wanting this for consistency reasons. But understanding the true scope of the issue will help me prioritize this in relation to the release roadmap that is floating in my head somewhere. 😄

[raycohen]

The downside of doing this is that it means users cannot make use of the module context as the module's context is not available inside an arrow function.

You're not required to use QUnit module context, and the module docs even discuss using lexical context instead of module context:

see where it says

It might be more convenient to use JavaScript's own lexical scope instead:

Personally I feel that module context is preferable to lexical scope, particularly for tests with lots of nested modules that alter setup values, where lexical scope can run into issues.

[platinumazure]

I'm open to having a way to enforce this. I think to avoid confusion, let's have a new rule for this no-arrow-module-definitons or similar.

What are we doing for module hooks (beforeEach, etc.)? Do we have a rule for those (maybe under no-arrow-tests)? Should we catch those too?

There's a part of me that wishes we had a catch-all rule no-arrow-callbacks or similar, and that we deprecate no-arrow-tests. But deprecations are a pain for users to deal with.

Thoughts?

[raycohen]

If you access this in any arrow function where the context is tied to a module and not to a test, you can get into trouble. It doesn't even have to be a callback to a QUnit method.

https://codepen.io/raycohen/pen/mdONpgB

module('top', function () {
  this.count = 1;

  // arrow function
  const setCount = (newValue) => { this.count = newValue; };
  // to fix, change above line to
  // this.setCount = function(newCount) { this.count = newValue; }

  module('inner 1', function (hooks) {
    hooks.beforeEach(function () {
      // has no impact on this test's context
      setCount(5);
    });
    test('author thinks count is 5', function (assert) {
      assert.equal(this.count, 5, 'fails, count is 1');
    });
  });
});​

[platinumazure]

If you access this in any arrow function where the context is tied to a module and not to a test, you can get into trouble. It doesn't even have to be a callback to a QUnit method.

So this seems like a larger scope than I had originally understood.

It seems as though you want to flag the following:

1. Any arrow function that is a module callback
2. Any arrow function within a module callback, but not within a test, which references this
   1. What about arrow functions that don't use this? Are the bindings clear enough in those cases?

Am I understanding correctly?

[raycohen]

Yes and even an arrow function passed as a module callback would be safe as long as it doesn't access this.

For thoroughness we'd need to check recursively through nested arrow functions to see if this is accessed.

module('example', function() {
  this.updateName = (value) => {
    doStuff().then(() => { 
      this.name = value; // should cause us to warn that `updateName` should not be an arrow function
    });
  };
});

This rule could have two modes - mode 1 is disallow arrow callbacks accessing this and mode 2 is disallow all* arrow callbacks. Mode 2 for teams that want the extra consistency and not to need to switch from arrow function to regular function upon introduction of a reference to this.

*all functions passed as QUnit module/test/hook callbacks, and as immediate children of a module callback