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

lib: refactor primordials.uncurryThis #36221

Closed
wants to merge 1 commit into from
Closed

lib: refactor primordials.uncurryThis #36221

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 5 additions & 11 deletions lib/internal/per_context/primordials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,11 @@
// `primordials.Object` where `primordials` is a lexical variable passed
// by the native module compiler.

const ReflectApply = Reflect.apply;

// This function is borrowed from the function with the same name on V8 Extras'
// `utils` object. V8 implements Reflect.apply very efficiently in conjunction
// with the spread syntax, such that no additional special case is needed for
// function calls w/o arguments.
// Refs: https://github.com/v8/v8/blob/d6ead37d265d7215cf9c5f768f279e21bd170212/src/js/prologue.js#L152-L156
function uncurryThis(func) {
return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}

// `uncurryThis` is equivalent to `func => Function.prototype.call.bind(func)`.
// It is using `call.bind(bind, call)` to avoid using `Function.prototype.bind`
// after it may have been mutated by users.
const { bind, call } = Function.prototype;
const uncurryThis = call.bind(bind, call);
Copy link
Member

Choose a reason for hiding this comment

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

not that this isn't a very cool application, but wouldn't this be a bit clearer?

const uncurryThis = (f) => Function.prototype.call.bind(f)

Copy link
Contributor Author

@aduh95 aduh95 Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is that Function.prototype.bind can be modified by the user. I could add a comment explaining it though. Do you think that would be enough?

Suggested change
const uncurryThis = call.bind(bind, call);
// The following line is equivalent to `const uncurryThis = f => call.bind(f);`.
const uncurryThis = call.bind(bind, call);

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I guess once you factor it out you just end up with call.bind(bind, call). A comment would be good then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL.

primordials.uncurryThis = uncurryThis;

function copyProps(src, dest) {
Expand Down