-
Notifications
You must be signed in to change notification settings - Fork 4
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
Documentation incorrect #2
Comments
The documentation isn't worded correctly, but I believe the rule behaves as expected. I intend to write a blog post that describes the issue more accurately, soon. Thanks for pushing back RE: docs. We do need to clean this up. 👍 |
Hey Kyle. Before I address the issue, I just want to say how bizarre and humbling it is to have two of my JS heroes comment on one of my repos. I began learning JavaScript only 6 months ago, so I'm sure you can imagine how crazy this feels! Back to the issue at hand - I will definitely review this tomorrow and the issue you linked in Babel to update the examples and introduction to be accurate. Thanks so much for taking the time to comment, and I apologize for the poorly worded documentation. |
@getify It's easy to make this mistake, since typical function expressions can use the specified function name inside the function without a lexical binding. For example: const bar = function foo () { console.log(typeof foo); };
bar(); // logs: 'function'
foo(); // no lexical 'foo' exists:
// ReferenceError: foo is not defined @johnstonbl01 I've written up a gist that more accurately describes the issue using the |
👍 For the record:
function foo(x) {
return obj.foo(x);
}
var obj = {
foo(x) {
if (x < 10) return foo(x * 2); // `foo` here comes from the function, not the concise method
return x;
}
};
obj.foo(4); // 16 |
Hey guys. Below is the change I'm proposing to the docs. I spent some time this morning re-reading your statements here, and reviewing the Babel issue again. I think I'm on the same page now, and I apologize for not completely getting it. Indeed as Kyle mentioned, I thought the original issue was with the name inferrence and not necesarily the lexical name binding. That was my mistake. Luckily, on some level I did understand the issue with the code and I believe that the rule logic is sound. @ericelliott - let me know when you've completed that blog post, and I'll link to it as well in the overview. OverviewIn ES6, compact methods and unnamed function expression assignments within object literals do not create a lexical identification (name) binding that corresponds to the function name identifier for recursion or event binding. The compact method syntax will not be an appropriate option for these types of solutions, and a named function expression should be used instead. This custom ESLint rule will identify instances where a function name is being called and a lexical identifier is unavailable within a compact object literal. More information on this can be found: Note - Tests are provided in the repo, but not necessary for installation or use of the rule. Example ChangesExample 1In the example below, 1 error is generated because foo is being called recursively when there is no lexical name binding for the const bar = {
name: 'Bar',
types: [
{ f: 'function' },
{ n: 'number' }
],
foo (f, n) { // this function will not have any lexical binding for recursive calls
if (typeof f === 'function') {
f();
} else {
throw new Error('foo: A Function is required.');
}
n -= 1;
if (!n) {
return undefined;
}
return foo(f, n); // error on this line
}
};
bar.foo(() => {console.log('baz');}, 3);
//baz
//ReferenceError: foo is not defined Example 2In this example, no errors are generated because the function expression explicitly defines a lexical identifier. const bar = {
name: 'Bar',
types: [
{ f: 'function' },
{ n: 'number' }
],
foo: function foo (f, n) { // this function explicitly defines a lexical name for the method
if (typeof f === 'function') {
f();
} else {
throw new Error('foo: A Function is required.');
}
n -= 1;
if (!n) {
return undefined;
}
return foo(f, n);
}
};
bar.foo(() => {console.log('baz');}, 3);
//baz
//baz
//baz Updated Error MessageI propose changing the current error message:
to
If you think it should be something more descriptive, please let me know. Error vs WarningI'm happy to change the docs to recommend setting the value of the rule to 1 (for warning) rather than 2 (for error). However, the Lastly - thanks to both of you for the help and guidance. It's really been a great learning experience. @getify - No worries on the original tone. I knew where you were coming from. 👍 |
Looks great to me!
I don't know if this is possible, but I'd suggest:
|
One more proposal - why suggesting to completely change the way method is defined instead of showing warning/error on call itself and suggesting to change |
@RReverser |
@ericelliott It's much less likely to be mistake than when someone doesn't pass |
@RReverser The argument here is which form is less likely to lead to code that doesn't work as expected. |
@ericelliott Do you suggest to also provide ESLint rule that restricts using |
Guys - let's keep it civil and on-topic, please. @RReverser Please keep in mind that this is a custom rule, and there's no plan for this to be integrated into the standard ESLint rules. If you don't agree with the way it is written, then it's possible to create your own to use or not use this one at all. That's the great thing about ESLint, in my opinion. @getify I think that should be possible, but I need to dig in and have a look. I'll push the README changes later today and get back to you on the warning / error functionality. |
@johnstonbl01 Of course I do understand that - I'm just pointing to other possible mistakes that this linting rule might accidentally push other devs towards (like not preserving context in object method when calling it recursively). Sorry if polluting the thread, I made my point so will stop here :) |
Thanks, @RReverser . :) |
Regarding the But if there's no other |
I don't think the rule needs any logic changes. We can adequately handle either case by changing the rule text:
For the sake of simplicity, let's assume the reader knows enough JS to figure out the rest. |
@ericelliott Sounds good to me! |
Ok, guys. Here are the updates:
Closing this issue for now - thanks for the input, everyone! |
[Edit: clarified and adjusted tone of my wording]
"Every form of function expression assignment in ES6 infers a name that can be used inside the function for recursion."
That statement is false. The function
name
inference has nothing to do with the ability to make lexical name recursive calls. The inference is assigning aname
property to the function object, not giving the function object a named binding in the surrounding (or owned) lexical environment (which would be required for recursion).The code snippet also reinforces this incorrect information:
That
foo(f,i)
call will be an error, as there will not be afoo
lexical identifier referencing the function. There is no such thing as inferring a lexical name. Thefoo
lexical name will exist at that line of code, but it'll be pointing to thefoo
object (not function) declared on the first line of the snippet. As such, aTypeError
will be thrown because you're trying to call a non-function value.Concise methods do not have lexical name bindings and thus such a lexical name cannot be used, being that it doesn't exist, for recursion or any other self-referential purpose.
The text was updated successfully, but these errors were encountered: