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.

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

Refactor FunctionLoader so that it returns a cloned function each time #564

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

ejizba
Copy link
Contributor

@ejizba ejizba commented Mar 29, 2022

Related to #553, people will be able to mess with the function more easily, so I want to make sure they can't affect the function across invocations.

Interestingly, we were already cloning the function in getEntryPoint as a part of setting the this arg. I just moved the logic so that it happens during getFunc instead of load and used the slightly cleaner bind instead of apply.

Copy link
Contributor

@hossam-nasr hossam-nasr left a comment

Choose a reason for hiding this comment

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

Seems like some magic to me with the bind business. How were we setting this appropriately before without using bind? Regardless, I don't want to block on this :)

@ejizba
Copy link
Contributor Author

ejizba commented Apr 8, 2022

Before

return function () { return f.apply(obj, arguments); };

After

return loadedFunction.func.bind(loadedFunction.thisArg);

If I were to use the old names, it would look like this:

return f.bind(obj);

The main benefit of bind is you can get rid of the wrapper function() {} stuff. Otherwise, I feel like bind and apply are both pretty magic-y so I don't think it makes much of a difference.

@ejizba ejizba merged commit 6be9f0d into v3.x Apr 8, 2022
@ejizba ejizba deleted the ej/funcLoad branch April 8, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants