Docs: Completely re-write allowUnboundThis option (fixes #8950) #9077

webdevdaemon · 2017-08-05T23:05:45Z

What is the purpose of this pull request? (put an "X" next to item)

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
For this PR (My FIRST PR btw... so go easy on me!), I did a re-write of the documentation
for the "prefer-arrow-callback" rule. per issue 8950, I only worked on the allowUnboundThis option description. I added a small intro for the option, and tried to be as clear and concise as possible in describing the option's behavior when set to true or false.

If or when this PR is approved and merged, I hope to rewrite the entire documentation page for "prefer-arrow-callback" to ensure continuity of voice, and to provide a bit more clarity as to WHAT greater concept/problem this rule addresses, and WHY one would want to integrate this rule into their linting process.

Is there anything you'd like reviewers to focus on?
allowUnboundThis:

Is my description accurate?
Is the format i finally decided upon an effective way to communicate this rule's intent?
Is the format acceptable for use within eslint's documentation

I GREATLY appreciate this opportunity. It has ALREADY been a valuable learning experience and has clarified a ton about how this whole open source thing actually operates. I look forward to contributing some actual code sometime in the near future!

Thanks again, looking forward to your input,
Charles M.

eslintbot · 2017-08-05T23:05:46Z

LGTM

mention-bot · 2017-08-05T23:05:46Z

@webdevdaemon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @IanVS and @BYK to be potential reviewers.

not-an-aardvark

Thanks for the pull request! I left a few minor suggestions for making the section a bit easier to read.

not-an-aardvark · 2017-08-08T07:30:01Z

docs/rules/prefer-arrow-callback.md

@@ -55,7 +55,19 @@ foo(function bar() {});

 ### allowUnboundThis

-This is a `boolean` option and it is `true` by default. When set to `false`, this option allows the use of `this` without restriction and checks for dynamically assigned `this` values such as when using `Array.prototype.map` with a `context` argument. Normally, the rule will flag the use of `this` whenever a function does not use `bind()` to specify the value of `this` constantly.
+__ES6 Arrow Functions__, when used to describe a callback function, _automatically_ bind to the surrounding context/scope. This is preferable to the previous standard for _explicitly bound_ callback functions - Combining __ES5 Function Expressions__ with `bind()` and `this` to achieve similar behavior. For additional information on the differences between ES5 function expressions, and ES6 arrow functions - [__CLICK HERE__]('https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions').


I like the idea of adding a documentation link, but I would rather put it at the bottom of the page in a "further reading" section like this.

OK. I will follow the format of the linked page. I appreciate the input, you guys are awesome, and extremely helpful. Will clean everything up, and get back to you guys.

not-an-aardvark · 2017-08-08T07:32:05Z

docs/rules/prefer-arrow-callback.md

@@ -55,7 +55,19 @@ foo(function bar() {});

 ### allowUnboundThis

-This is a `boolean` option and it is `true` by default. When set to `false`, this option allows the use of `this` without restriction and checks for dynamically assigned `this` values such as when using `Array.prototype.map` with a `context` argument. Normally, the rule will flag the use of `this` whenever a function does not use `bind()` to specify the value of `this` constantly.
+__ES6 Arrow Functions__, when used to describe a callback function, _automatically_ bind to the surrounding context/scope. This is preferable to the previous standard for _explicitly bound_ callback functions - Combining __ES5 Function Expressions__ with `bind()` and `this` to achieve similar behavior. For additional information on the differences between ES5 function expressions, and ES6 arrow functions - [__CLICK HERE__]('https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions').


This section seems useful to describe the distinction between arrow functions and function expressions, but it seems like it would fit better as part of the general rule description rather than as part of the allowUnboundThis option description.

Also, I have a few minor nitpicks:

The section mentions "ES5 function expressions", in contrast to ES6 arrow functions, but this seems a bit inaccurate because function expressions have been around since before ES5.

I don't feel strongly about this, but I'm not a big fan of having so much bold text.

First off, thanks for the PR!

Agreed with @not-an-aardvark - the bold/italics reads a little bit aggressively to me 😅

Gotcha - will address these asap.

I agree on the description - that is why I wanted to possibly rewrite the rest of the rule as well while putting that bit of extra info into the rule's introduction. I of course just stuck to what y'all had approved me to work on.

But ya, I will make these fixes and get back to you.

NOOB question: Do i submit another PR, or is there a way to edit this PR? if there is a way to edit it - just say so, and I will look up how to do it - no need to waste precious time on rookie magic over here ;-)

Oh, and thx a ton for the input/critique. I truly appreciate it.

Do i submit another PR, or is there a way to edit this PR? if there is a way to edit it?

If you create another git commit on your issue-8950-PR and then run git push again, this PR will automatically be updated with the latest changes.

Another valuable lesson - I'm totally leveling up right now!

not-an-aardvark · 2017-08-08T07:38:31Z

docs/rules/prefer-arrow-callback.md

+Behavior when `false` (STRICT):
+
+- __ALLOWED__: Callbacks containing `this` are not allowed unless described using ES6 Arrow Function syntax.
+- __NOT ALLOWED__: Any use of `this` from within an ES5 Function Expression callback, whether _explicitly bound_ or not.


This section seems overly verbose to me -- I don't think it's necessary to have "allowed/not allowed" sections for each option value. I think the behavior of the option could be described more clearly in one or two sentences, with something like:

With {"allowUnboundThis": true} (default), the rule allows function expressions in callbacks containing this, provided that they are not explicitly bound with .bind(this). With {"allowUnboundThis": false}, the rule disallows function expressions in callbacks entirely.

…eslint#8950)

eslintbot · 2017-08-14T13:33:58Z

LGTM

webdevdaemon · 2017-08-14T14:04:03Z

OK, sorry for the wait - life got busy there for a sec...

The introductory piece about es6 arrow function behavior, that I had originally placed in the allowUnboundThis description, Has been moved to the top of the page - along with a little bit more context for those that may not have a firm grasp n ES6 yet, and/or how it differs from ES3/5.
I relocated the related/additional info link to the bottom of the page, I tried to make it as identical to the example provided to me as possible.
I went out on a limb, and decided to go for it, and rewrite the entire rule rather than just the option description outlined by the issue. My edits were already finding their way into other sections of the rule description (the intro, and the further reading link), and after trying for a while to make the two distinct voices sound more cohesive, I decided to just rewrite the whole thing. I made sure to apply any relevant points from your first round of critiques, to the rest of the text that I altered. I also kept the examples untouched, and tried to preserve as much of the original author's information as possible as well. I apologize if this was inappropriate.
I removed all of the obnoxious bold and italics - I think that my setup's styles for previewing markdown are too mild. Eslint's docs present text decorations more dramatically, so this was solid advice. Thanks for catching what I couldn't on this one!
I think I found a solid balance between too-verbose, and too-sparse in describing the rule's behavior/intended use. Let me know if I missed, or added something that needs to be changed.
I changed all occurences of "ES5 function expression" to plain old "function expression" - for obvious reasons. 'Twas a silly mistake.

That's about it, Let me know what you guys think...

Thanks again

…eslint#8950)

eslintbot · 2017-08-14T14:10:37Z

LGTM

ilyavolodin · 2017-08-19T02:03:07Z

docs/rules/prefer-arrow-callback.md


-Arrow functions are suited to callbacks, because:
+Compared to function expressions, arrow functions are better-suited for describing callbacks or function arguments.


Could you re-word this sentence somehow? It feels too forceful and opinionated the way it's currently written. We try very hard to avoid forcing our opinions on users.

Yes. Will do.

Done, let me know if this works!

Thank you

eslintbot · 2017-08-20T02:41:39Z

LGTM

kaicataldo

Thanks for all your work on this. Docs are the often neglected part of OSS projects, and the work you're doing is extremely helpful and very important!

I have a few more suggestions, but this is looking really good :)

kaicataldo · 2017-08-20T03:11:47Z

docs/rules/prefer-arrow-callback.md


- `this` keywords in arrow functions bind to the upper scope's.
- The notation of the arrow function is shorter than function expression's.
+For example, arrow functions are bound to their surrounding scope/context automatically. This is an upgrade from using `bind()` and `this` to explicitly bind a function expression, which was the pre-ES6 standard for achieving similar scoping behavior.


"This is an upgrade from using bind() and this" feels too opinionated for me. Maybe something like, "This makes them a less verbose alternative to using bind() and this" or even just "This allows them to be used as an alternative to using bind() and this", since you explain why below?

kaicataldo · 2017-08-20T03:12:28Z

docs/rules/prefer-arrow-callback.md


-Arrow functions are suited to callbacks, because:
+Arrow functions can be an attractive alternative to function expressions, when describing callbacks or function arguments.


This is very nitpicky, but I think a comma is unnecessary here: function expressions, when

kaicataldo · 2017-08-20T03:14:22Z

docs/rules/prefer-arrow-callback.md


 ## Rule Details

-This rule is aimed to flag usage of function expressions in an argument list.
+This rule identifies function expressions being used as callbacks or function arguments, where upgrading to an arrow function would achieve identical results.


"where upgrading to an arrow function" doesn't feel like right here. How about "where using an arrow function" instead?

kaicataldo · 2017-08-20T03:14:55Z

docs/rules/prefer-arrow-callback.md

-This rule is aimed to flag usage of function expressions in an argument list.
+This rule identifies function expressions being used as callbacks or function arguments, where upgrading to an arrow function would achieve identical results.
+
+Instances where an arrow function would not achieve identical results will be ignored.


An example of the cases this rule ignores would be helpful!

kaicataldo · 2017-08-20T03:15:50Z

docs/rules/prefer-arrow-callback.md


 ### allowNamedFunctions

-This is a `boolean` option and it is `false` by default. When set to `true`, the rule doesn't warn on named functions used as callbacks.
+This option accepts a `boolean` value and is `false` by default. When `false`, any named functions used as callbacks will produce a flag.


Might be clearer if we replace and is with that is.

kaicataldo · 2017-08-20T03:17:11Z

docs/rules/prefer-arrow-callback.md


 foo(function bar() {});
 ```

 ### allowUnboundThis

-This is a `boolean` option and it is `true` by default. When set to `false`, this option allows the use of `this` without restriction and checks for dynamically assigned `this` values such as when using `Array.prototype.map` with a `context` argument. Normally, the rule will flag the use of `this` whenever a function does not use `bind()` to specify the value of `this` constantly.
+By default `{ "allowUnboundThis": true }`, this option will allow function expressions containing `this` to be used as callbacks - as long as the function in question has not been explicitly bound.


Probably don't need the dash :)

eslintbot · 2017-08-22T13:46:58Z

LGTM

webdevdaemon · 2017-08-22T13:47:07Z

OK, I addressed all of your concerns, which led to a few additional edits. Specifically, I edited the comments to be more informative and supportive of the rule/options. I hope this didn't just introduce more problems... I made sure that I am 100% content with this draft on my end, so that any additional ideas from your end can be quickly integrated and pushed back up for final approval if necessary.

Looking forward to your responses on this one.

Thanks!

kaicataldo

This is very close! Thanks for sticking with us on this :)

kaicataldo · 2017-08-22T15:26:16Z

docs/rules/prefer-arrow-callback.md


 ## Rule Details

-This rule is aimed to flag usage of function expressions in an argument list.
+This rule locates function expressions that have been submitted as function arguments or callbacks. If any of these function expressions could  achieved by an arrow function will produce an error.


Tiny nit: extra space in could achieved by

The sentence also feels a little awkward. Maybe "If any of these function expression callbacks can be replaced with arrow function expressions, the rule will produce an error."?

kaicataldo · 2017-08-22T15:26:39Z

docs/rules/prefer-arrow-callback.md

 ```

-The following patterns are not considered problems:
+Instances where an arrow function would not produce identical results will  be ignored.


Tiny nit: extra space in will be ignored

kaicataldo · 2017-08-22T15:28:56Z

docs/rules/prefer-arrow-callback.md

+/* eslint-env es6 */
+
+foo(a => a); // OK
+// arrow function callback


Do you think it might be clearer to put the comments above the code sample? Not a huge blocker - just something I'm used to seeing.

kaicataldo · 2017-08-22T15:34:05Z

docs/rules/prefer-arrow-callback.md

@@ -1,77 +1,100 @@
-# Suggest using arrow functions as callbacks. (prefer-arrow-callback)
+# Suggest using ES6 arrow functions to describe callbacks (prefer-arrow-callback)


The word describe doesn't quite feel right to me. What do you think of making it something like Suggest using ES6 arrow functions for callbacks (prefer-arrow-callback)?

We should also make sure this matches the metadata property in the rule itself!: https://github.com/eslint/eslint/blob/master/lib/rules/prefer-arrow-callback.js#L136

Can I change the metadata property as part of this same PR? or do I need to create a new one to make them match?

Please do change the metadata on this same PR if you can. Thanks!

kaicataldo · 2017-08-22T15:34:54Z

docs/rules/prefer-arrow-callback.md


-Arrow functions are suited to callbacks, because:
+Arrow functions can be an attractive alternative to function expressions when describing callbacks or function arguments.


Same comment as above. Maybe Arrow functions can be an attractive alternative to function expressions for callbacks or function arguments.?

eslintbot · 2017-08-28T07:46:22Z

LGTM

webdevdaemon · 2017-08-28T07:47:18Z

OK, edits made, let me know if line 15 sounds any better - if not I will just take your suggestion word-for-word and call it good :-)

kaicataldo

If you wouldn't mind updating the metadata for the rule itself, that would be great.

A few very minor nitpicks. The reason I think clarifying these words is important is that ESLint is used by many who are not native English speakers, and the simpler we can make the documentation, the more accessible we make it for everyone!

Thanks for working on this :)

kaicataldo · 2017-08-28T14:42:29Z

docs/rules/prefer-arrow-callback.md


 ## Rule Details

-This rule is aimed to flag usage of function expressions in an argument list.
+This rule locates function expressions that have been submitted as function arguments or callbacks. An error will be produced for any that could be replaced by an arrow function without changing the result.


"submitted" doesn't quite feel right to me here. Maybe something like:

This rule locates function expressions that are used as function arguments or callbacks

kaicataldo · 2017-08-28T14:44:18Z

docs/rules/prefer-arrow-callback.md


 ### allowNamedFunctions

-This is a `boolean` option and it is `false` by default. When set to `true`, the rule doesn't warn on named functions used as callbacks.
+By default `{ "allowNamedFunctions": false }`, this `boolean` option prohibits the use of named functions when describing callbacks or function arguments.


"describing" doesn't quite feel right here. Maybe something like:

By default { "allowNamedFunctions": false }, this boolean option prohibits the use of named functions as callbacks or function arguments.

kaicataldo · 2017-08-28T14:44:49Z

docs/rules/prefer-arrow-callback.md

-This is a `boolean` option and it is `true` by default. When set to `false`, this option allows the use of `this` without restriction and checks for dynamically assigned `this` values such as when using `Array.prototype.map` with a `context` argument. Normally, the rule will flag the use of `this` whenever a function does not use `bind()` to specify the value of `this` constantly.
+By default `{ "allowUnboundThis": true }`, this `boolean` option will allow function expressions containing `this` to be used as callbacks, as long as the function in question has not been explicitly bound.
+
+When set to `false` this option prohibits the use of function expressions to describe callbacks or function arguments entirely, without exception.


"describe" doesn't quite feel right here. Maybe something like:

When set to false this option prohibits the use of function expressions as callbacks or function arguments entirely, without exception.

eslintbot · 2017-08-30T23:39:09Z

LGTM

webdevdaemon · 2017-08-31T00:00:54Z

Alright, made those edits. I also addressed the metadata as requested. I really appreciate the opportunity to learn about the PR process & etiquette. Git used to be such a mystery - github & PRs even more so. Thanks so much to everyone involved for helping me through my first PR.

Let me know if there is anything else you need from me on this one!

Thanks

not-an-aardvark

Looks good to me. Thanks for contributing!

eslintbot · 2017-08-31T00:06:11Z

LGTM

kaicataldo

Other than one tiny typo this LGTM. Thanks for contributing!

kaicataldo · 2017-08-31T16:53:20Z

docs/rules/prefer-arrow-callback.md


-foo(a => a);
-foo(function*() { yield; });
+// generator as callbackcccx


Typo: callbackcccx

eslintbot · 2017-09-01T21:02:38Z

LGTM

ilyavolodin · 2017-09-01T21:54:09Z

Thanks for the PR! It's great to have people contribute docs fixes, not everyone thinks it's helpful, but it really is.

kaicataldo · 2017-09-03T17:33:51Z

Yes! And really appreciate you collaborating with us through all the feedback - I'm pretty happy where we landed with this!

webdevdaemon · 2017-09-04T09:46:42Z

The pleasure was all mine, friends.

I'll likely be in touch soon to help you guys out further. Thanks so much for holding my hand through this one.

Cheers until next time!

webdevdaemon added 4 commits August 5, 2017 14:01

Docs: Completely re-write allowUnboundThis option (fixes 8950)

00d0733

Docs: Completely re-write allowUnboundThis option (fixes 8950)

bf5bff4

prep for new branch

7e60929

Docs: Completely re-write allowUnboundThis option (fixes 8950)

6885fca

not-an-aardvark suggested changes Aug 8, 2017

View reviewed changes

not-an-aardvark changed the title ~~Issue 8950 pr - Docs update - fixes 8950~~ Docs: Completely re-write allowUnboundThis option (fixes #8950) Aug 8, 2017

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules labels Aug 8, 2017

Docs: Edit format, less bold text, add intro, less-verbose desc (fixes …

de1f864

…eslint#8950)

Docs: Edit format, less bold text, add intro, less-verbose desc (fixes …

3eca8d2

…eslint#8950)

ilyavolodin suggested changes Aug 19, 2017

View reviewed changes

Docs: rephrase opening statement to be more passive (fixes 8950)

7a9d923

kaicataldo suggested changes Aug 20, 2017

View reviewed changes

Docs: addressed concerns, edited code examples (fixes 8950)

be86168

kaicataldo suggested changes Aug 22, 2017

View reviewed changes

Docs: further edits to description. (fixes 8950)

ca32a82

kaicataldo reviewed Aug 28, 2017

View reviewed changes

Docs: edit for clarity, edit metadata to match docs(fixes 8950)

cf053cc

not-an-aardvark approved these changes Aug 31, 2017

View reviewed changes

Docs: edit docs & meta description to fit test criteria(fixes 8950)

f790902

kaicataldo approved these changes Aug 31, 2017

View reviewed changes

ilyavolodin approved these changes Sep 1, 2017

View reviewed changes

Fix small typo in prefer-arrow-callback.md

c233d58

not-an-aardvark merged commit 73815f6 into eslint:master Sep 1, 2017

eslint-deprecated bot locked and limited conversation to collaborators Mar 1, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 1, 2018


		Arrow functions are suited to callbacks, because:
		Compared to function expressions, arrow functions are better-suited for describing callbacks or function arguments.


		Arrow functions are suited to callbacks, because:
		Arrow functions can be an attractive alternative to function expressions, when describing callbacks or function arguments.

		@@ -1,77 +1,100 @@
		# Suggest using arrow functions as callbacks. (prefer-arrow-callback)
		# Suggest using ES6 arrow functions to describe callbacks (prefer-arrow-callback)

Docs: Completely re-write allowUnboundThis option (fixes #8950) #9077

Docs: Completely re-write allowUnboundThis option (fixes #8950) #9077

Conversation

webdevdaemon commented Aug 5, 2017

eslintbot commented Aug 5, 2017

mention-bot commented Aug 5, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 14, 2017

webdevdaemon commented Aug 14, 2017

eslintbot commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 20, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 22, 2017

webdevdaemon commented Aug 22, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 28, 2017

webdevdaemon commented Aug 28, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 30, 2017

webdevdaemon commented Aug 31, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

eslintbot commented Aug 31, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 1, 2017

ilyavolodin commented Sep 1, 2017

kaicataldo commented Sep 3, 2017

webdevdaemon commented Sep 4, 2017

kaicataldo Aug 28, 2017 •

edited

Loading