stylelint / stylelint

A mighty CSS linter that helps you avoid errors and enforce conventions.
https://stylelint.io
MIT License
10.97k stars 936 forks source link

Fix require time performance #2454

Open chris-morgan opened 7 years ago

chris-morgan commented 7 years ago

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

Loading stylelint (require('stylelint')) is very slow. It takes over a second for me when there’s nothing else installed, and when there are various other things installed it increases (e.g. two seconds).

I care about this because in a Makefile world where you spin up the tooling for each file, this is a >1 second delay per file which rapidly changes from unpleasant to awful.

Steps to reproduce:

$ cd $(mktemp -d)
$ yarn add stylelint
[…]
$ node -e "s=Date.now();require('stylelint');console.log(Date.now()-s)"
1183
$ node -e "s=Date.now();require('stylelint');console.log(Date.now()-s)"
1128
$ node -e "s=Date.now();require('stylelint');console.log(Date.now()-s)"
1155

Which rule, if any, is this issue related to?

n/a

What CSS is needed to reproduce this issue?

n/a

What stylelint configuration is needed to reproduce this issue?

n/a

Which version of stylelint are you using?

7.9.0

How are you running stylelint: CLI, PostCSS plugin, Node API?

n/a

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

n/a

What did you expect to happen?

require('stylelint') to be fast (like, well under 100ms).

What actually happened (e.g. what warnings or errors you are getting)?

It took a long time to import stylelint, a second or two.

vankop commented 4 years ago

Without the above exclusions, I'm getting a minified bundle-size of 3.5mb and require-time of 120ms.

I think without bundling time is the same. Last time I checked it was around ~140ms (but some fixes for loading time wasn't merged yet)

jeddy3 commented 4 years ago

I think without bundling time is the same. Last time I checked it was around ~140ms (but some fixes for loading time wasn't merged yet)

That's correct. The gains of bundling are only apparent when the syntaxes are bundled separately. This makes sense as the bundling replaces lazyloading.

I should have made that clearer in my original post.

vankop commented 4 years ago

What do you think about marking rules as externals? Why we need to bundle them eagerly?

jeddy3 commented 4 years ago

Why we need to bundle them eagerly?

For the browser (https://github.com/stylelint/stylelint/issues/3935), e.g. tools like JS Fiddle.

hudochenkov commented 4 years ago

Lodash might not go away, because other dependencies have it:

├─┬ @stylelint/postcss-css-in-js@0.37.1
│ └─┬ @babel/core@7.9.0
│   ├─┬ @babel/generator@7.9.5
│   │ └── lodash@4.17.15  deduped
│   ├─┬ @babel/helper-module-transforms@7.9.0
│   │ └── lodash@4.17.15  deduped
│   ├─┬ @babel/traverse@7.9.5
│   │ └── lodash@4.17.15  deduped
│   ├─┬ @babel/types@7.9.5
│   │ └── lodash@4.17.15  deduped
│   └── lodash@4.17.15  deduped
├─┬ postcss-reporter@6.0.1
│ └── lodash@4.17.15  deduped
└─┬ table@5.4.6
  └── lodash@4.17.15  deduped

Instead we'll have much more dependencies. One for every lodash function.

jeddy3 commented 4 years ago

Lodash might not go away, because other dependencies have it:

Interesting. I wonder what ncc's support for tree-shaking is like?

It looks like:

I've been meaning to remove the dependency on postcss-reporter. We only use a tiny util from it in the string reporter.

I think it's worth looking into further as lodash is a significant chunk of the bundle weight at over half a meg and it impacts require time.

I should have made that clearer in my original post.

I should have also been clearer that reducing the require time from 120 to 80 is only half the story. It looks like a bundled stylelint (with the separate syntaxes) is twice as fast to run, which would benefit everyone but especially extension authors and people in the Makefile world, like the original poster.

jeddy3 commented 4 years ago

I tried it locally and we can cherry-pick the lodash function we use. We'd still have a single lodash dependency, but we'd cherry-pick functions from it (as table and babel already do).

For example:

// package.json
{
  "dependencies": {
    "lodash": "^4.17.15",
  }
}
// stringFormatter.js
const _escapeRegExp = require('lodash/escapeRegExp');
const _flatMap = require('lodash/flatMap');
const _sortBy = require('lodash/sortBy');
const _uniqBy = require('lodash/uniqBy');

It knocks off a chunk of weight from the browser bundle.

hudochenkov commented 4 years ago

Can we use https://github.com/lodash/babel-plugin-lodash to automate this for bundle?

jeddy3 commented 4 years ago

Can we use https://github.com/lodash/babel-plugin-lodash to automate this for bundle?

The plugin only supports ES Modules syntax. I used the plugin to rewrite the source files (having first replaced all the lodash requires with imports) and then I converted them back commonjs when I did my local test.

m-allanson commented 4 years ago

There is an ESLint rule that we can use to ensure method imports from lodash. See eslint-plugin-lodash/import-scope.

I have written a codemod we can use to make this change across the stylelint repos. See m-allanson/codemod-lodash-requires. I'm planning to work on this once https://github.com/stylelint/stylelint/pull/4796 is finished.

m-allanson commented 4 years ago

This is a useful tool for analysing require times: require-so-slow.

Here's the output from the latest version of stylelint, running on my machine (requires Chrome): devtools timeline viewer (stylelint.trace)

Screenshot 2020-06-06 at 12 42 44

Create your own trace with npx require-so-slow stylelint.

jeddy3 commented 2 years ago

ESBuild can bundle the v14 branch without issue. If anyone fancies it, they're welcome to investigate bundling for the 14.0.0 release and open a pull request. It's something we'll need to investigate later down the line anyway for https://github.com/stylelint/stylelint/issues/5291.

I suspect we'll want to bundle:

Stylelint should remain installable from GitHub. We'll need to investigate how this is done with np, perhaps using npm lifecycle scripts and the --content & --test-script np flags?

We'll want to weigh up the performance gains against any additional maintenance burden. Investigating bundling and opening a pull request will give us the opportunity to assess that.

You can give ESBuild a go with:

./node_modules/.bin/esbuild ./lib/index.js --bundle --outfile=out/index.js --format=cjs --platform=node --minify --metafile=lib.json

Which will result in a 814.3kb bundle with consistently sub-100ms require times (down from the 200ms for 13.x and 130ms from an unbundled 14.x).

If you like, you can upload the generated metafile to https://www.bundle-buddy.com to see a breakdown. Thanks to all the hard refactoring work done (including the removal of lodash) the breakdown looks clean.