Skip to content

Drop function-bind dependency with saving safety#17

Merged
tarruda merged 2 commits intotarruda:masterfrom
zloirock:master
Oct 3, 2023
Merged

Drop function-bind dependency with saving safety#17
tarruda merged 2 commits intotarruda:masterfrom
zloirock:master

Conversation

@zloirock
Copy link
Copy Markdown
Contributor

That reduces the minified size of the package from 1KB to a hundred bytes.

The argument about protection from delete Function.prototype.call does not work here since in ES5+ engines .call is not used after loading, but in ES3 engines function-bind use .call internally anyway.

The argument that "most likely you have function-bind in your dependencies chain" also does not work - I haven't it in any other places.

Copy link
Copy Markdown
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Most people most likely already have it; it’s true that “most likely” means some people wont.

You’re also correct that in pre-ES5 engines, reliance on .call is unavoidable.

I don’t think this is a helpful change (worth the churn), but i agree it’s not a harmful/broken one.

Comment thread src/index.js Outdated
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 17, 2021

"Most likely" should mean more than a half. I'm sure that it is not. Even has, which depends on function-bind and generates significant part traffic on it, have even more downloads than function-bind. Even if it's somehow used by half of the users, it's not an argument to add 1KB of size to bundles of another half of users.

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 17, 2021

https://npmjs.com/call-bind (18m) also depends on function-bind (23m, about the same as has), which might mean that there’s significant overlap between call-bind users and has users (78%, much more than half) - but it also definitely means it’s pretty difficult to look at download numbers and reliably come to any conclusion, in either direction.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 17, 2021

Anyway, it's not crucial since this PR removes 1KB from bundles that haven't other function-bind dependencies, but adds just 18 bytes to bundles that have it (after removing useless here 'use strict' and direct Function.prototype it will be even less).

@zloirock
Copy link
Copy Markdown
Contributor Author

@tarruda

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Dec 2, 2021

1.5 months and some tenths of millions harmful function-bind downloads from NPM later it's still ignored...

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Dec 2, 2021

bytes aren't harm.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Dec 2, 2021

Yes, they are extremely use 😂

@zloirock

This comment has been minimized.

@ljharb

This comment has been minimized.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Dec 3, 2021

@ljharb unlike my approach, this your approach steals some milliseconds from the life of almost each internet user every day.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 3, 2023

Merged, thanks @zloirock

Pushed 1.0.4 without the dependency

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

@tarruda this is a breaking change; it should have been a v2, and v1 needs to keep the dependency.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

@ljharb Why is this a breaking change? The API and behavior are still the same

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

@tarruda not in an ES3 environment where Function bind doesn't exist. That's the purpose of the dependency.

If this dep is not restored, I'll have to replace has in all of my packages, since it will no longer serve its purpose :-/

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

@ljharb Isn´t this ternary operator already checking for the presence of function.bind and providing a fallback in case it is not available?

image

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

In particular, it means it's no longer robust against delete Function.prototype.call after the module is loaded - which the previous code was.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

call is saved in the module scope, even if it is deleted, it won´t affect the module:

image

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

call.call on line 7 only works if it's still on Function.prototype, so it will, indeed, affect the module when bind isn't present at module load time, and call is deleted later.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 4, 2023

How many .call and .apply are used in your polyfill on runtime? -) All these arguments have been discussed a long time ago.

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

I've stated my piece; i consider this a breaking change, and if it remains I'll have to drop has as a dep, which would be a lot of churn and move a lot of downloads away from it.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

@ljharb Do you have a test case which breaks on the new version and passes on the previous one?

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

I'm traveling right now, so I'll need time to come up with it - suddenly merging and releasing a PR with nearly 2 years of silence is pretty jarring - but either way, on some level the effort may be better spent just migrating to something I can rely on for stability.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

@ljharb the arguments presented for merging this PR made sense to me, and I didn't see any counter arguments.

If you have a test case showing that this change breaks compatibility, I would be happy to fix it.

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 4, 2023

@tarruda sure. run:

delete Function.prototype.bind;
var has = require("has@1.0.3");
Object.defineProperty(Function.prototype.call, 'call', { value: undefined });
has({}, 'toString') === false && has({toString:1}, 'toString');

vs

delete Function.prototype.bind;
var has = require("has@1.0.4");
Object.defineProperty(Function.prototype.call, 'call', { value: undefined });
has({}, 'toString') === false && has({toString:1}, 'toString');

in https://npm.runkit.com/has.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 4, 2023

Let's replace

Object.defineProperty(Function.prototype.call, 'call', { value: undefined });

to

Object.defineProperty(Array.prototype.slice, 'call', { value: undefined });

or

delete Function.prototype.apply;

or to many other alternative options. The new version is significantly more protected.

If you see this as a breaking change, then a significant part of patch releases of your projects are also breaking.

@tarruda
Copy link
Copy Markdown
Owner

tarruda commented Oct 4, 2023

@ljharb I'm fine with breaking compatibility scenarios where the user is actively trying to destroy his JS context. We could protect against this situation by storing call.call locally in the module scope, but is this worth it? I mean, if the user wants to destroy his JS context, why would it matter if the has module is still working or not?

If you have a more realistic test case showing that 1.0.4 breaks compatibility with 1.0.3, then I will fix it. If this response not satisfy you, then feel free to stop using has in your packages.

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 5, 2023

I suppose I'll have to make my own package, then.

It matters because no realistic JS program has a single author's code running in it - every environment has untrusted code running, and thus everyone should be protecting against malice.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 5, 2023

And for the sake of this, you insist on more vulnerable code. Everything is logical -)

@ljharb
Copy link
Copy Markdown
Contributor

ljharb commented Oct 5, 2023

You are more than welcome to file issues on a project you find vulnerable to robustness concerns, as I do.

@zloirock
Copy link
Copy Markdown
Contributor Author

zloirock commented Oct 5, 2023

Why, if without it this package will work better anyway and it won't be possible to implement such a level of safety with function-bind that has significant unnecessary logic with its own safety problems?

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.

3 participants