-
Notifications
You must be signed in to change notification settings - Fork 5
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
Motivation beyond defensiveness against mutation of globals? #8
Comments
Regarding if this improves defense against global mutation: Yes, the #7 pull request, which I have just merged, adds discussion about two ways this improves defense (first, performance by avoiding wrapping with bound functions; second, developer ergonomics). The ergonomics problem could be debatable, but the performance problem is pretty clear (see nodejs/node#38248). (CC: @ljharb, @bmeck.) Regarding if there are other use cases: Yes, there is are “generic methods” and “individually importable methods” use cases (#6). I plan to write a section about this within the next few weeks. Generic methods are already an idiomatic pattern in JavaScript (e.g., |
That performance problem holds in the current implementation of V8, nowhere else - not even necessarily in V8 in the future. I would not regard that as a reason for adding new syntax. (V8 optimizes common patterns; they haven't optimized this one because it's not common. It's not like it's fundamentally impossible to optimize. But if it's not common enough to optimize in V8, it's surely not common enough to warrant new syntax.)
|
After further discussion from Matrix, it’s clear that, in order to have any chance at Stage 1, I need to drastically change the focus of the explainer. I need to focus less on global protection and more on Function.prototype.call and bind being generally common, as well as tree-shakable, individually importable methods being desirable.
(Further clarification on this performance problem being specific to V8 would be appreciated from anyone on the Node team. If it is indeed optimized by other engines, then I should remove the section discussing that “problem”. CC: @ljharb, @aduh95, whomever else available.) |
@bakkot V8 does actually try to hot path these coding patterns at the behest of node and perf E.G. https://bugs.chromium.org/p/v8/issues/detail?id=9702 also not all engines optimize this so I'm not sure I understand the claim that it only affects V8. |
Do any engines optimize this? |
@ljharb V8 tries to, I couldn't find any other engine that inlines by unwrapping the bound function after some fiddling in the console. |
I meant that Node is built on V8 and so its performance issues only give you information about what is optimized in V8 rather than in any other engine. |
I’ve revamped the explainer to now focus on the commonality and clunkiness of |
@bakkot: I’m going to close this, since the explainer got completely revamped…but I’d be very interested to know if you think the explainer has become more compelling than it had been before. Cheers. |
The readme gives as the sole motivation for this proposal the desire to be defensive against mutation of
Function.prototype
.I don't find this motivation nearly compelling enough to justify new syntax. If you want to run in an untrusted environment already need to be saving any other prototype methods, as the readme notes, even given this syntax:
Once you're doing that, the additional overhead of saving the various
Function.prototype
methods is negligible. That is, code which wants to be defensive can (and indeed generally does) already writeSo it seems to me that this syntax does relatively little to improve this relatively obscure use case. As such, it doesn't seem to me to even approach the bar for adding new syntax, at least not on the strength of the motivations given in the readme. Are there other motivations?
The text was updated successfully, but these errors were encountered: