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

Simplify dependency tree #5012

Closed
gajus opened this Issue Jan 20, 2016 · 15 comments

Comments

Projects
None yet
6 participants
@gajus
Copy link
Contributor

gajus commented Jan 20, 2016

#5000 (comment)

In my view, one of the flaws in ESLint setup is the use of "modular" lodash NPM packages.

You could get rid of these dependencies:

"es6-map": "^0.1.3",
"escape-string-regexp": "^1.0.2",
"handlebars": "^4.0.5",
"lodash.clonedeep": "^3.0.1",
"lodash.merge": "^3.3.2",
"lodash.omit": "^3.1.0",
"object-assign": "^4.0.1",
"xml-escape": "~1.0.0"
by simply adding lodash@4 that has all of those features (and no sub-dependencies. The sub-dependency tree of these dependencies is huge).

And there is no reason whatsoever to use "modular" lodash packages (given that ESLint is a development tool).>

#5000 (comment)

@gajus if you're willing to go through and make those changes to use lodash@4 instead of the independent ones, we can take a look.

#5000 (comment)

I am on it.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jan 20, 2016

@gajus Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 20, 2016

@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Jan 20, 2016

Regarding removal of "jsonlint" package. Your current implementation does not even work, e.g. try replacing the contents of conf/json-schema-schema.json with invalid JSON. The reason it does not work is because jsonlint does not support multiple file input, zaach/jsonlint#77. Your made condition was validating only the first JSON file.

@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Jan 20, 2016

The removed packages are:

"es6-map": "^0.1.3",
"escape-string-regexp": "^1.0.2",
"handlebars": "^4.0.5",
"lodash.clonedeep": "^3.0.1",
"lodash.merge": "^3.3.2",
"lodash.omit": "^3.1.0",
"object-assign": "^4.0.1",
"jsonlint": "^1.6.2",
"xml-escape": "~1.0.0"

Added packages:

"lodash": "^4.0.0",
@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 20, 2016

It would be good if you can share the before and after output of npm perf run on the project.
Thanks for doing this.

@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Jan 20, 2016

issue5012 branch:

npm run perf

> eslint@2.0.0-beta.1 perf /Users/gajus/Documents/dev/gajus/eslint
> node Makefile.js perf

CPU Speed is 2600 with multiplier 7500000
Performance Run #1:  1767.5270150000001ms
Performance Run #2:  1734.082259ms
Performance Run #3:  1706.810887ms
Performance Run #4:  1720.917011ms
Performance Run #5:  1736.409199ms
Performance budget ok:  1734.082259ms (limit: 2884.6153846153848ms)


Load performance Run #1:  125.223919ms
Load performance Run #2:  126.405084ms
Load performance Run #3:  126.158342ms
Load performance Run #4:  131.567232ms
Load performance Run #5:  127.39286ms

Load Performance median :  126.405084ms

master branch:

npm run perf

> eslint@2.0.0-beta.1 perf /Users/gajus/Documents/dev/gajus/eslint
> node Makefile.js perf

CPU Speed is 2600 with multiplier 7500000
Performance Run #1:  1715.774711ms
Performance Run #2:  1688.8597869999999ms
Performance Run #3:  1692.669535ms
Performance Run #4:  1680.084062ms
Performance Run #5:  1683.229413ms
Performance budget ok:  1688.8597869999999ms (limit: 2884.6153846153848ms)


Load performance Run #1:  -1ms
Load performance Run #2:  -1ms
Load performance Run #3:  -1ms
Load performance Run #4:  -1ms
Load performance Run #5:  -1ms

Load Performance median :  -1ms

I am not familiar with the expected behaviour/ output of the perf program. The master branch perf seems off.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 21, 2016

@gyandeeps any idea why the load perf would error out like that?

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 21, 2016

it works on my machine, that means when the branches were switched that npm install was not done again (I guess)

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 21, 2016

Here is the run on windows 7 with node 4.2.4 :

Master:

> eslint@2.0.0-beta.1 perf C:\Users\gs025879\Documents\webstrom\eslint
> node Makefile.js perf

CPU Speed is 2793 with multiplier 7500000
Performance Run #1:  1948.548346ms
Performance Run #2:  1978.786296ms
Performance Run #3:  1985.663256ms
Performance Run #4:  1986.728835ms
Performance Run #5:  1967.682577ms
Performance budget ok:  1978.786296ms (limit: 2685.2846401718584ms)


Load performance Run #1:  202.345226ms
Load performance Run #2:  204.4236ms
Load performance Run #3:  202.018991ms
Load performance Run #4:  202.37455ms
Load performance Run #5:  214.471637ms

Load Performance median :  202.37455ms

Gajus's branch (PR): Its like ~30 commits behind current master.

> eslint@2.0.0-beta.1 perf C:\Users\gs025879\Documents\webstrom\gajus
> node Makefile.js perf

CPU Speed is 2793 with multiplier 7500000
Performance Run #1:  1990.800174ms
Performance Run #2:  1965.773552ms
Performance Run #3:  1966.777184ms
Performance Run #4:  1984.285371ms
Performance Run #5:  1975.718954ms
Performance budget ok:  1975.718954ms (limit: 2685.2846401718584ms)


Load performance Run #1:  182.135886ms
Load performance Run #2:  182.355454ms
Load performance Run #3:  183.421398ms
Load performance Run #4:  189.756222ms
Load performance Run #5:  180.843776ms

Load Performance median :  182.355454ms
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Jan 21, 2016

What about performance for multiple files?

@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Jan 22, 2016

What about performance for multiple files?

@mysticatea Not aware how to run a test for multiple files. If you tell me how, I will be happy to do it.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Jan 22, 2016

@gajus Thank you. I was modifying the target here to check performance for multiple files before.
It's a bit of inconvenience, though.

@mourner

This comment has been minimized.

Copy link
Contributor

mourner commented Feb 5, 2016

Up all up for replacing non-lodash modules with lodash, but can you elaborate on the advantage of including the whole lodash instead of a few modules? It's a 4MB download.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Feb 6, 2016

How did you calculate 4MB for lodash?

@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Feb 6, 2016

I can only assume:

~  cd ~/Desktop
➜  Desktop  mkdir test
➜  Desktop  cd testtest  npm install lodash
/Users/gajus/Desktop/test
└── lodash@4.2.1

npm WARN ENOENT ENOENT: no such file or directory, open '/Users/gajus/Desktop/test/package.json'
npm WARN EPACKAGEJSON test No description
npm WARN EPACKAGEJSON test No repository field.
npm WARN EPACKAGEJSON test No README data
npm WARN EPACKAGEJSON test No license field.
➜  test  du -sh
4.0M    .test
@gajus

This comment has been minimized.

Copy link
Contributor Author

gajus commented Feb 6, 2016

It's a 4MB download.

It is not. You are downloading a .tar.gz, which is 234K.

ilyavolodin added a commit that referenced this issue Feb 8, 2016

Merge pull request #5015 from gajus/issue5012
Update: Replace several dependencies with lodash (fixes #5012)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.