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

remove is-negative-zero #154

Closed
wants to merge 2 commits into from
Closed

remove is-negative-zero #154

wants to merge 2 commits into from

Conversation

bartekleon
Copy link

No description provided.

@bartekleon bartekleon closed this Jun 2, 2024
@ljharb
Copy link
Owner

ljharb commented Jun 3, 2024

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

@bartekleon
Copy link
Author

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

@ljharb
Copy link
Owner

ljharb commented Jun 3, 2024

What the purpose of a fork?

@bartekleon
Copy link
Author

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
Copy link
Owner

ljharb commented Jun 3, 2024

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
Copy link
Author

bartekleon commented Jun 3, 2024

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.

bartekleon#1 (without that second bad commit) is proof of concept, although your call is last

@ljharb
Copy link
Owner

ljharb commented Jun 3, 2024

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
Copy link
Author

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
Copy link
Owner

ljharb commented Jun 3, 2024

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

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.

2 participants