-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Naming arrow functions #6433
Comments
This would also be nice to have for arrow functions used as class methods/properties. private handleDocumentClick = (...) => {
...
} |
Can you clarify this part? |
Note that we can't safely name class methods -- a reference to |
Consulted with @bterlson and we think this is a spec-affecting behavior, specifically the places where the abstract Regular function expressions have very noncomplaint behavior in current runtimes and should probably just be left alone (specifically, Edge, Chrome, and Node all produce an own property of |
Hi @RyanCavanaugh, answering your question... as far as I know in ES6 if an arrow function is defined on the right-hand-side of an assignment expression, the engine will take the name on the left-hand-side and set the arrow function's ".name" based on that Thanks |
Hi @RyanCavanaugh, could you explain the SetFunctionName operation? I couldn't fully grasp why it should be an issue. I tried this in chrome.
Generated typescript:
So it seems under the covers v8 makes nice named anonymous functions. |
@basarat It didn't happen for me in v50.0.2661.102m, but I just upgraded to v51.0.2704.63m and now it's doing that. |
PRs welcomed |
I will attempt to make a PR in following days or weeks, depending on how difficult it turns out to be. ;) |
@mhegazy For clarification - should PR for this issue fix all naming issues according to ES6 spec (naming arrow function object properties etc...), or only this one case of Also, should it only target arrow functions or regular anonymous functions - Also, is it ok to open work in progress PR to make sure changes go in good direction and get some help from community (as a newbie contributor)? |
I would say all lambdas/function expressions should behave as such. feel free to send us incomplete PRs if you want to get early feedback. please tag @rbuckton on your PRs. |
See also: #6757. I notice the PR hasn't been merged, is anybody working on this? |
I worked on it for some time but I am currently not. |
using TypeScript 2.3.3, issue hasn't been solved, #6757 is a dup. |
@OfficerHalf doesn't react use babel for compilation anyway? So what TS transpilation looks like should not really matter.. @DanielRosenwasser In order to fix this babel mangles the variable names beyond recognition (ex). Wouldn't a partial fix here for functions that do not reference themselves in the body be good enough ? Adding a name to those will not cause (*) any shadowing issues and it will cover most common scenarios while not mangling the output. On the other hand this solution will require an extra pass over all arrow function to determine if they do reference themselves, which may impact performance so it might just make sense to label this as will not fix and closing it 🤷♀️ (*) as far as I can tell |
Sure, if you use CRA it uses babel. But you don't have to - if you use |
I'm inclined to agree with @dragomirtitian that we should just Won't Fix this due to the rapidly-evaporating ecosystem of ES5-only runtimes. The actual impact here is quite minimal compared to the sheer ugliness of the required emit. |
IE 11 isn’t vanishing particularly rapidly. |
Agreed - over 50% of traffic I see in my particular industry comes from IE11. We can't deprecate it yet since some customers are unable to upgrade even if they wanted to. |
Would it be acceptable if the names were set to something "sufficiently close" to the original name? The point in that being that it would make debugging easier even if it's not the correct behavior. So in const foo = () => {
} we would rewrite that to var foo = function foo_1() {
} |
while that's certainly better than outputting no name at all, or a mangled name, that seems like a short-term workaround pending an actual fix. |
Why would A be easier than the desired B?
var foo = function foo_1() {
}
var foo = function foo() {
} |
@basarat Picking a name that does not conflict with anything else would remove the need to rewrite variable names if the body of the function references the the variable. Ex:
If you give the first function the name So you end up having to do a lot of variable name rewrites, which get pretty ugly pretty fast: ex |
Variable names aren’t observable, however, while function names are. |
Can someone implement this please? |
This is a [bug in typescript](microsoft/TypeScript#6433)
This is a (bug in typescript)[microsoft/TypeScript#6433]
This is a bug in typescript (microsoft/TypeScript#6433)
This appears to be fixed now, no? |
@jedmao no. The relevant test would be const n = () => "hello"; which should produce var n = function n() { return "hello"; };
^ under |
At this point I'm not sure its entirely necessary to fix this, except in possibly a very small number of cases. If you emit to ES5/ES3, but are running in an ES2015+ host, function names are automatically assigned from their variables using NamedEvaluation (which evolved from variable declaration evaluation in ES2015). This means that if you have the following typescript: const f = () => {};
console.log(f.name); And we transpile it to the following ES5: var f = function() {};
console.log(f.name); Running that transpiled output will print Broadly emulating NamedEvaluation for ES5 emit would artificially inflate the overall emitted file size, for little benefit. In fact, the only place where I could see it have value is for downleveled exports and class fields, as property assignments do not call NamedEvaluation: // ts
export const f = () => {};
console.log(f.name);
class C {
x = () => {};
};
console.log(new C().x.name);
// es5 (commonjs)
exports.f = function () {};
console.log(exports.f.name);
var C = (function () {
function C() {
this.x = function() {};
}
return C;
})();
console.log(new C().x.name); In ES2015+ with native ES modules, the above TS code prints This could possibly be resolved by leaning on NamedEvaluation for exports: // es5 (commonjs)
var f = function() {}
exports.f = f;
console.log(exports.f.name); // prints 'f' in an ES2015+ host Classes are a bit trickier if there is a naming conflict (though we could rename local variables to accommodate): // ts
class C {
x = () => {};
constructor() {
var x = 1;
}
};
// es5 (commonjs)
// option 1, rename locals
var C = (function () {
function C() {
var x = function() {};
this.x = x;
var x_1 = 1; // renamed local `x` to `x_1`
}
return C;
})();
// option 2, leverage _NamedEvaluation_ using object literals
var C = (function () {
function C() {
this.x = { x: function () {} }.x; // NamedEvaluation gives the name `x` to the function
var x = 1; // local not renamed
}
return C;
})(); One of the reasons to lean on NamedEvaluation is that we can't blindly add a name to a function expression as that can introduce scope conflicts. The name introduced in the function expression is locally scoped to the function expression body. Consider the two following cases: // case 1
export let fib = (i: number) => i === 1 ? 0 : i === 2 ? 1 : fib(i - 1) + fib(i - 2);
const fib2 = fib;
fib = i => { console.log("i:", i); return fib2(i); };
console.log("# case 1");
console.log("result:", fib(3));
// case 2
function f(s: string) { console.log(s); }
class C {
f = (s: string) => { f("Hello " + s); }
}
console.log();
console.log("# case 2");
new C().f("Alice"); If you emit to ESNext using native ES modules, you would get the following results:
If we were to emit to ES5 and inject names into the functions, the output would change due to scope conflicts:
The reason for this would be the resulting emit: exports.fib = function fib(i) { return i === 1 ? 0 : i === 2 ? 1 : fib(i - 1) + fib(i - 2) };
var fib2 = fib;
fib = function (i) { console.log("i:", i); return fib2(i); };
function f(s) { console.log(s); }
var C = (function () {
function C() {
this.f = function f(s) { f("Hello " + s); };
}
return C;
})();
new C().f("Alice"); In case 1, the In case 2, the |
When it was standardized isn't important; it was present in every version of every web browser except IE, and it's web reality that code expects it to work. It's also polyfillable in IE 9+, with https://npmjs.com/function.prototype.name, and there's lots of websites that rely on that working, including airbnb.com (when i last checked, at least). |
Every product will have a set of tradeoffs it has to make, and prioritizing the work to do this "by the book" is work with a likely tail of bugs and emit size implications. I can't speak for others who are on this issue, but I tend to agree with Ron that the people who would benefit from this have shrunk a lot as a group. For example:
Windows 10 (released late July 2015) actually makes it hard to access IE11. In fact, it will automatically punt you to Edge unless you disable a setting within Edge. But for the sake of it, here's Airbnb running in IE11: abnb-ie11.mp4 |
@DanielRosenwasser that's a totally reasonable assessment; my pushback was against trying to justify not doing the work as a result of when something was standardized. Regardless of what's done by default, why not offer this as a config option, for those who want to emit correct code? |
This is not practical to fix: the costs far outweigh the benefits. |
@sandersn even behind an option? |
Hi there,
Not sure whether this has been already mentioned before (I would be surpirsed if it never was)... Wouldn't it be a good idea to name arrow functions, which is what apparently ES6 does?. Think about the benefits of this in the stacktrace. It's quite useful to have a "named" function in there, instead of an anonymous one ;-)
So, this:
would be transpiled down to:
Just to mention that Babel does it like that ;-)
Thanks.
The text was updated successfully, but these errors were encountered: