-
Notifications
You must be signed in to change notification settings - Fork 5.6k
fix(cli/rt/console): fix inspection of Function #7930
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @kt3k
I'll wait for @nayeemrmn approval before landing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
const gf = function gf() {}; | ||
Reflect.setPrototypeOf(gf, null); | ||
assertEquals(stringify(gf), "[Function: gf]"); | ||
const agf = function agf() {}; | ||
Reflect.setPrototypeOf(agf, null); | ||
assertEquals(stringify(agf), "[Function: agf]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is already merged, but was this intended to test generator functions and async generator functions? (function* gf() {}
and async function* agf() {}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solson good catch, could you open an issue about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function* gf() {}
andasync function* agf() {}
Are you saying they should have their own output? That's just declaration-side syntactic sugar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm saying they should be declared that way here in the test, as in const gf = function* gf() {};
to match async function af
above, presumably to test this "set prototype to null
" manipulation on all 4 of Function
, AsyncFunction
, GeneratorFunction
, and AsyncGeneratorFunction
. Currently, gf
and agf
are just testing the same thing as f
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR fixes the inspection of functions. The current implementation gets the name of the type of the function from
f.__proto__.constructor.name
, and it throws when the prototype is set to null as described in #7633. This PR checks the prototype before accessing its constructor name and uses the generic name 'Function' if the prototype is not available.fixes #7633