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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Sep 1, 2017
Merged

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

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
00d0733
Docs: Completely re-write allowUnboundThis option (fixes 8950)
webdevdaemon Aug 5, 2017
bf5bff4
Docs: Completely re-write allowUnboundThis option (fixes 8950)
webdevdaemon Aug 5, 2017
7e60929
prep for new branch
webdevdaemon Aug 5, 2017
6885fca
Docs: Completely re-write allowUnboundThis option (fixes 8950)
webdevdaemon Aug 5, 2017
de1f864
Docs: Edit format, less bold text, add intro, less-verbose desc (fixe…
webdevdaemon Aug 14, 2017
3eca8d2
Docs: Edit format, less bold text, add intro, less-verbose desc (fixe…
webdevdaemon Aug 14, 2017
7a9d923
Docs: rephrase opening statement to be more passive (fixes 8950)
webdevdaemon Aug 20, 2017
be86168
Docs: addressed concerns, edited code examples (fixes 8950)
webdevdaemon Aug 22, 2017
ca32a82
Docs: further edits to description. (fixes 8950)
webdevdaemon Aug 28, 2017
cf053cc
Docs: edit for clarity, edit metadata to match docs(fixes 8950)
webdevdaemon Aug 30, 2017
f790902
Docs: edit docs & meta description to fit test criteria(fixes 8950)
webdevdaemon Aug 31, 2017
c233d58
Fix small typo in prefer-arrow-callback.md
not-an-aardvark Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion docs/rules/prefer-arrow-callback.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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').
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thanks for the PR!

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


By default `{"allowUnboundThis": true}`, eslint _loosely_ enforces this option. When `false`, eslint _strictly_ enforces the option.

Behavior when `true` (LOOSE):

- __ALLOWED__: ES5 Function Expression callbacks containing the `this` keyword, __AS LONG AS__ the function in question __HAS NOT__ been _explicitly bound_.
- __NOT ALLOWED__: Callbacks described using ES5 Function Expression syntax that contains use of the `this` keyword, and has been _explicitly bound_.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Examples of **incorrect** code for the `{ "allowUnboundThis": false }` option:

Expand Down