ljharb / es-abstract

ECMAScript spec abstract operations.
MIT License
115 stars 30 forks source link

remove is-negative-zero #154

Closed bartekleon closed 3 months ago

ljharb commented 3 months ago

Please don't submit unsolicited PRs making large refactors to open source projects without discussing them with maintainers first.

bartekleon commented 3 months ago

Wanted to make PR to my own fork, but by default it set main. Sorry about it.

ljharb commented 3 months ago

What the purpose of a fork?

bartekleon commented 3 months ago

I was curious about if is-negative-zero is used/how it's used, since it seems you are using the package is-negative-zero while also having same/similar code in the repositorium - helpers/isNegativeZero.js or something like this. They are equivalent in functionality afaik.

ljharb commented 3 months ago

hm, yeah that's a good point - i do both use the dep and have the helper. It'd be better (for bundle size/deduping) for all the callsites to just use the dep directly, and for the helper to re-export the dep for compat. Is that something you'd like to convert this PR into doing?

bartekleon commented 3 months ago

I mean, yeah I could. I don't see how it would change bundle size though since it's just removing one line of code for a library that has 30KB. IMO it should be the other way around, removing dependency and leaving the helper - since it's basically the same line, but without dependency. Even without considering size etc, its an extra call to NPM to download it.

https://github.com/bartekleon/es-abstract/pull/1 (without that second bad commit) is proof of concept, although your call is last

ljharb commented 3 months ago

Disk size of a package has zero bearing on bundle size - by using a dep instead of the helper, the relevant line of code (which might already be in your tree via the dep) will only appear once, instead of twice. (also, bundle size has nothing to do with download size - download size is irrelevant, only "what runs in the actual application" matters)

bartekleon commented 3 months ago

Nowadays it matters less since disk space and interner speed are much bigger than they used to and yes, even if removed from this package, it might still be downloaded by other project. (at the same time now imagine 100 or 200 dependencies like this, which ultimately makes let's say 6MB to download [considering 30kb/packages], since we are also downloading tests and other "garbage" [stuff we don't actually use]. And now we have 10-100 projects on our PC... That adds up fast).

That's why I said it's ultimately your call.

But that being said maybe one thing that actually popped up in my head would be if you could remove some files from what is published to NPM (like tests for example, since they are not typically used in code). But probably discussion about it should be left for some issue/other way of communication, not PR comments :)

ljharb commented 3 months ago

Tests and whatnot are used - npm explore foo && npm install && npm test should always work.