-
Notifications
You must be signed in to change notification settings - Fork 108
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
SetFunctionName - problems and inconsistencies #502
Comments
It's more than that; |
Well I was presuming that the suggested fix were to be added. Though actually even with that fix the usage is still broken, in particular i.e. At present: Object.defineProperty(parseInt, "name", { configurable: true, value: "NOT_PARSE_INT" });
console.log(parseInt.toString()); will always print:
regardless of what you change However if the aforementioned proposed "fix" is done, then actually this suddenly becomes possible: function decorate() {
return parseInt;
}
class SomeClass {
@decorate
NOT_PARSE_INT() {
}
}
// Now prints "function NOT_PARSE_INT() { [native function] }"
console.log(parseInt.toString()); |
Thinking about this more with regards to stack traces I'm actually thinking it might be an outright bad idea. For example if a decorated method were to have it's name automatically set then an error thrown in the original function makes the same name appear for otherwise distinct functions. e.g. If we do: function decorate(method) {
return function decoratedMethod(...args) {
// ... such and such
return method.call(this, ...args);
}
}
class Foo {
@decorate
method() {
if (someCondition()) {
throw new Error(`some error`);
}
// ...
}
} then stack traces will appear like:
Is this really more desirable than?:
|
Sure, it would need to also only set the InitialName slot when it was previously unset - and arguably it's already missing an assertion that it's not previously set. |
I'm in agreement that setting the name automatically is a bad idea. Yes, there's a negative impact to the debugging experience if the function name isn't set, but that's already the case when "monkey patching" existing methods. It may be cumbersome, but I would still much rather a developer be explicit about naming if it is important to their use case, i.e.: function setFunctionName(f, name, prefix) {
if (typeof name === "symbol") name = `[${name.description}]`;
if (prefix) name = `${prefix} ${name}`;
Object.defineProperty(f, "name", { configurable: true, value: name });
return f;
}
function decorate(target, context) {
return setFunctionName(function () { return target.apply(this, arguments); });
}
// or
function decorate(target, context) {
// hacky way to assign name
return { [context.name]: function () { return target.apply(this, arguments); } }[context.name];
} |
For some comparable behaviour in another language, in Python names are not carried over automatically, rather there's a builtin facility to copy the name (and docstring) over: import functools
def decorate(method):
@functools.wraps(method)
def decorated_method(self):
return method()
return new_method
class Test:
@decorate
def method():
raise Exception("some error")
test = Test()
# The name is updated
print(test.method.__name__)
test.method() Interestingly though in this example, within a stack trace the original name of the function is still shown even though
Given the fairly flexible ways engines display stack traces in JS already, I suppose it would be already be allowed for engines to show more useful information anyway. e.g. Perhaps engines could show something like the following:
|
This has been resolved, |
So currently method, method/getter/setter methods call
SetFunctionName
to set the name of those methods/getters/setters (though this currently broken it's presumed this will fixed).However setting this unconditionally has some problems, namely:
"name"
is not configurable (e.g. a frozen function)Further setting names isn't even consistent, the spec does it for methods, getters and setters but doesn't for auto-accessors or classes:
In most regards I think setting all these names is probably the best default, though it should really fail gracefully if the function
"name"
is not configurable (e.g. frozen).Though it would be nice to have an API to change the name if possible:
The text was updated successfully, but these errors were encountered: