Skip to content

fix: replace is-number dep with a one-liner#17

Merged
paulmillr merged 3 commits into
micromatch:masterfrom
v1rtl:remove-is-number
Jul 30, 2024
Merged

fix: replace is-number dep with a one-liner#17
paulmillr merged 3 commits into
micromatch:masterfrom
v1rtl:remove-is-number

Conversation

@v1rtl

@v1rtl v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor

This PR replaces is-number package with a one-liner with identical code. It passes all the tests (npm run test).

This tiny change saves 440GB weekly traffic:

Package size report
===================

Package info for "to-regex-range@5.0.1": 33 kB
  Released: 2019-04-07 06:04:37.03 +0000 UTC (277w2d ago)
  Downloads last week: 43,837,006
  Estimated traffic last week: 1.5 TB

Removed dependencies:
  - is-number@7.0.0: 10 kB (30.06%)
    Downloads last week: 43,875,245
    Downloads last week from "to-regex-range@5.0.1": 43,837,006 (99.91%)
    Estimated traffic last week: 440 GB
    Estimated traffic from "to-regex-range@5.0.1": 440 GB (99.91%)

Estimated package size: 33 kB → 23 kB (69.94%)
Estimated traffic over a week: 1.5 TB → 1.0 TB (440 GB saved)

Comment thread index.js Outdated
Co-authored-by: Sam Martin <privatesmartin@gmail.com>
@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

Another option is using typeof min === 'number' but I need to investigate whether anybody is supplying string-as-number input for this lib

@VigoTes

VigoTes commented Jul 30, 2024

Copy link
Copy Markdown

Great job

@jonschlinkert

Copy link
Copy Markdown
Member

but I need to investigate whether anybody is supplying string-as-number input for this lib

It's almost always going to be a string. This library was created to use with parsed values.

Does this pass all of the is-number unit tests?

@mrlkn

mrlkn commented Jul 30, 2024

Copy link
Copy Markdown

Similar PR opened two years ago and closed by @jonschlinkert

You are single handedly contributing to the climate change for two years.

@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

but I need to investigate whether anybody is supplying string-as-number input for this lib

It's almost always going to be a string. This library was created to use with parsed values.

Does this pass all of the is-number unit tests?

I haven't tried it against is-number test suite but instead ran tests for this lib locally. in case its mostly expecting string input, I will not replace it with typeof

Comment thread index.js
@jonschlinkert

Copy link
Copy Markdown
Member

You are single handedly contributing to the climate change for two years.

This PR and this entire way of thinking is a waste of time and energy.

@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

guys let's be nice, the intent of this PR is to reduce network bandwidth because npm ships stuff as tarballs (and not source directly) and to reduce supply chain / dependency graph (it takes time to parse it + adds potential security risks), I have no political or any other reasons for this and don't approve of insulting people

no need to be rude to each other

@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

Does this pass all of the is-number unit tests?

happy to report that yes, it does: 111 passing (9ms)

@jonschlinkert

Copy link
Copy Markdown
Member

guys let's be nice, the intent of this PR is to reduce network bandwidth because npm ships stuff as tarballs (and not source directly) and to reduce supply chain / dependency graph

NPM doesn't bundle your code. NPM is a package manager. This is like saying you should remove your README because it saves bandwidth.

@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

guys let's be nice, the intent of this PR is to reduce network bandwidth because npm ships stuff as tarballs (and not source directly) and to reduce supply chain / dependency graph

NPM doesn't bundle your code. NPM is a package manager. This is like saying you should remove your README because it saves bandwidth.

npm doesn't bundle it, but it downloads the whole tarball, which wastes bandwidth (people still pay for internet) and slows down CI during installs. In a scale of one package it might not be noticeable, but when you got a lot of them (and you usually do) it slows things down monumentally

also yes, ideally a package should ship as small README as possible. there's 0 reasons for a library consumer to have it stored on their machine. only source files and package manifest matter

@jonschlinkert

jonschlinkert commented Jul 30, 2024

Copy link
Copy Markdown
Member

but it downloads the whole tarball

This is how development works. Nothing is optimized for production.

but when you got a lot of them (and you usually do) it slows things down monumentally

I'd rather have builds that take a little longer with code that works, than fast builds with shitty code.


To all of the people that gave this a thumbs down, first of all I was being felicitous. Second, you clearly didn't understand the point I was making: this PR and ones like it will increase bundle size for your END USERS.

As I stated above, nothing is optimized for PRODUCTION by the PACKAGE MANAGER. Reducing dependencies might make things download faster for you personally during dev, but there is 1) an exponential number of end users for every developer, 2) an exponential number of downloads of bundled production code for every one download of a package during development. Every time I see stats about download sizes like the one above it makes me cringe. If every dependency was a collection of micro-libraries, packages would be a fraction of the size they are today, because no code would be duplicated, and only the best variants of any given concept would be used. Instead, PRs like this one are making it increasingly likely that packages will be more bloated with more bugs over time.

@ineanto

ineanto commented Jul 30, 2024

Copy link
Copy Markdown

but it downloads the whole tarball

This is how development works. Nothing is optimized for production.

but when you got a lot of them (and you usually do) it slows things down monumentally

I'd rather have builds that take a little longer with code that works, than fast builds with shitty code.

I don't think it can be answered as simply as "this PR introduces shitty code". As demonstrated by the above post by @talentlessguy, the is-number test suite passes without any error. This PR is about taking an easy-to-make decision: reduce bandwith usage at the cost of litteraly nothing.

@v1rtl

v1rtl commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

but it downloads the whole tarball

This is how development works. Nothing is optimized for production.

but when you got a lot of them (and you usually do) it slows things down monumentally

I'd rather have builds that take a little longer with code that works, than fast builds with shitty code.

but it downloads the whole tarball

This is how development works. Nothing is optimized for production.

but when you got a lot of them (and you usually do) it slows things down monumentally

I'd rather have builds that take a little longer with code that works, than fast builds with shitty code.

...this is the same code as in is-number.... wtf

@churchianity

Copy link
Copy Markdown

but it downloads the whole tarball

This is how development works. Nothing is optimized for production.

but when you got a lot of them (and you usually do) it slows things down monumentally

I'd rather have builds that take a little longer with code that works, than fast builds with shitty code.

"shitty code" isn't a productive or substantive way to describe the changes in this, or any PR. If you have an issue with it, please describe it.

@paulmillr paulmillr merged commit b9fa118 into micromatch:master Jul 30, 2024
@doowb

doowb commented Jul 30, 2024

Copy link
Copy Markdown
Member

If you have an issue with it, please describe it.

I have a couple of issues with this:

  1. I was about to close it as a duplicate because in another PR, it was already stated that reducing dependencies alone is not a goal in this project. And that doing so, would cause even more work downstream.

  2. The original PR did not copy all of is-number, which may cause unforeseen issues later. There's a reason that is-number has been iterated on and refactored to be stable and reliable. By inlining that code (even if it is small), puts more maintenance burden on this library. If a bug is found and fixed in is-number, then this library and any other dependents get the benefit of that fix. Without using is-number as a dependency, we'd have to do more work to monitor the source dependency and update the code here.

As for bundle size... I agree with @jonschlinkert that your final build step can produce a compact bundled version of your application. Also, for CI builds, that bandwidth is most likely between (or even within) data centers.

@v1rtl v1rtl deleted the remove-is-number branch July 30, 2024 19:20
@paulmillr

Copy link
Copy Markdown
Contributor

I've merged it.

If a bug is found and fixed in is-number

Something as simple as is-number should not have a separate library.

Looking at the code, it's also misleading. It's not is-number. It is (js integer) number or a string which looks like js integer.

@43081j

43081j commented Jul 30, 2024

Copy link
Copy Markdown

just to add my two cents - its unlikely we'd do such a breaking change here, but i think really this library should accept strictly numbers and numbers-as-strings (things which Number(str) will suffice)

more for anyone seeing this and thinking of copying the code to other projects - maybe don't. just assert that typeof n === 'number' (and throw if it isn't) or, if you must support strings, something like typeof n === 'number' || !Number.isNaN(Number(n)).

@paulmillr paulmillr self-assigned this Jul 30, 2024
@micromatch micromatch deleted a comment from SuperchupuDev Jul 31, 2024
@micromatch micromatch deleted a comment from icymanred Jul 31, 2024
@micromatch micromatch deleted a comment from v1rtl Jul 31, 2024
@micromatch micromatch deleted a comment from icymanred Jul 31, 2024
@micromatch micromatch deleted a comment from ljharb Jul 31, 2024
@micromatch micromatch deleted a comment from SuperchupuDev Jul 31, 2024
@micromatch micromatch deleted a comment from v1rtl Jul 31, 2024
@micromatch micromatch locked as resolved and limited conversation to collaborators Jul 31, 2024
@jonschlinkert

Copy link
Copy Markdown
Member

Since I'm still getting notifications about this and other packages with is-number...

i think really this library should accept strictly numbers and numbers-as-strings (things which Number(str) will suffice)

That's why is-number was used! The fact that we even need to discuss it is more than enough justification to use a dependency for it.

Dependencies can REDUCE bundle size

  1. Package size is only relevant during development. Who cares if you save 20kb during dev? It's insane that people care about this, while using bloated 2Gb (literally) monstrosities like Gatsby without a care in the world.
  2. Using dependencies is more likely to reduce bundle size for end users. The bundled size of is-number is 411 bytes before it's minified. Every time someone does a PR like this one, we will obviously geometrically increase bundle sizes.

If you want to cut down on package size, go do PRs to remove tests, fixtures, benchmarks, docs, and any other garbage that ends up in node_modules but should have been excluded from the published package. Then, do PRs to use dependencies instead of inlined code for anything that's written more than once throughout the entire dependency tree. I'll say that again: do the exact opposite of what this PR does. Every time someone does a PR like this one, you are increasing the odds that the bundle size will geometrically grow over time. That is a mathematically provable.

Honestly things like AI autocompletion make it easier and easier to just inline something instead of using dependencies, or at least organizing that code in a central place, so it can be imported by other modules. But that's not a good thing. We're going to have increasingly bloated code, with inconsistent implementations of the same thing duplicated multiple times in one tree. But hopefully AI will solve that too...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.