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
Reduce install size by 30% #2876
base: master
Are you sure you want to change the base?
Conversation
These are supported in Node 6 and 7, respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin supports node 4, and inlining a polyfill is always a much much worse solution than using a dependency.
const fromEntries = Object.fromEntries || function fromEntries(entries) { | ||
const obj = {}; | ||
for (const [key, value] of entries) obj[key] = value; | ||
return obj; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlining a polyfill (one that's not matching the spec) is a nonstarter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not rewrite it to avoid the expensive polyfill then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polyfill isn't expensive, it's the cheapest possible way to correctly implement the method.
@@ -1,8 +1,5 @@ | |||
'use strict'; | |||
|
|||
const fromEntries = require('object.fromentries'); | |||
const entries = require('object.entries'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this plugin supports node 4, which lacks Object.entries, so we still need the polyfill.
const arrayIncludes = require('array-includes'); | ||
const values = require('object.values'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node 4 also lacks these functions.
Why would this plugin have any reason to support Node 4 when ESlint itself only supports Node 10+? That's a lot of baggage for no discernable gain. |
Because it also supports down to eslint 3, which supports node 4. The gain is backwards compatibility, and avoiding a semver-major bump. |
Have you evaluated the performance and disk usage tradeoff this is inherently making, or is this a purely philosophical stance? |
It's a dev tool; install speed impact is negligible and disk usage is irrelevant. |
I'm not sure that viewpoint is broadly shared. Developers have hundreds of projects on a machine, and this one is just under 8mb. Anecdotally, eslint-plugin-react accounts for more than 1GB of disk usage on my machine. |
I have almost 500 projects on my personal machine that are fully npm-installed, a handful of them using eslint-plugin-react, and the total size of the entire dir that holds them (including If the goal is to take up less disk space, I'm happy to discuss ways to do that on those polyfill repos - that would have far greater impact than removing them from one eslint plugin. |
That seems like a reasonable plan - I don't have exact numbers on the % impact of the polyfill repos, but estimating based on packagephobia seems to indicate they're at least 50% of the disk usage of this plugin. |
Here's a number for one included polyfill. 3.39mb. Would be good at least to not have duplicated monstrous packages. |
Maybe releasing library major version every 10 major version of node.js would be "stable enough". @ljharb how long do you plan to support node v4? |
Are there stats somewhere on the actual users of ESLint 3, node.js 4 that need the latest I understand that people could be using older versions of ESLint and node.js, but supporting runtimes that were released 7 years ago seems a bit overkill. Even more so that for people stuck on old versions of ESLint and node.js are also probably stuck on React 0.14 Releasing a new major version of |
@onigoetz react 18 works just fine on ancient versions of node, so that assumption doesn't hold. Also, the instant we shipped a bugfix or a new feature, the folks stuck on the previous major would be left in the lurch. That's a high cost, compared to the virtually zero cost of "a few extra packages". @chyzwar i maintain over 300 packages that still support node 0.4. So, "how long"? Probably forever. |
True, maybe it would suck /if/ those people are actually using Node 4 and ESLint 3 On another hand, your package has 12'500'000 downloads a week, according to packagephobia, one install is 3.77MB (Unzipped, so let's say half that when transfered) Which means (3.77/2)*12500000 = 23 562 500 MB = 23.5625 TB In other words, this PR is proposing to shave off 7.8 TB of data transfer per week. Here is an article written by |
Hi @yannickcr! I'm working on reducing the disk usage of a typical ESlint setup, and found a couple cases where this plugin can be slimmed down with no functional impact:
Object.entries
is supported in Node 6+ and does not need to be polyfilled.Object.fromEntries
is supported in Node 12+, and its usage here is minimal enough that an inline shim suffices.Array.prototype.includes
is supported in Node 6+Object.values
is supported in Node 7+