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

Implement caching #2293

Merged
merged 1 commit into from Mar 26, 2017

Conversation

7 participants
@sergesemashko
Contributor

sergesemashko commented Jan 27, 2017

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

#2270

Is there anything in the PR that needs further explanation?

UPDATED:
Tests are in progress done.
I decided to submit a PR for initial feedback before going too deep into woods with tests.

New dependencies:

Accounted for issues ESlint had with implementing cache:

@jeddy3, @davidtheclark, please, take a look

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jan 27, 2017

@sergesemashko Awesome, thanks for helping 👍

@IanVS

This comment has been minimized.

IanVS commented Feb 6, 2017

Looks like you did a great job researching the lessons we learned over at ESLint. I'm very excited to see this functionality land in stylelint. 👏

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 6, 2017

I have more free time this week to submit tests as well, I'm close

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 7, 2017

UPDATE: I added tests for cache feature.

Travis and appveyor are running out of memory in the very beginning, not sure why :/

On my local all tests are passing on node 7.5.0:

@ntwb, @evilebottnawi @davidtheclark, any ideas or suggestions?

@m-allanson

This comment has been minimized.

Member

m-allanson commented Feb 7, 2017

@sergesemashko this is an exciting feature, thanks for working on it!

When we first started using Jest we had some issues with CI failing due to running out of memory. It looks like that is what's causing the failures on this PR too.

I've tried running your branch locally and noticed that the tests take significantly longer to run when compared to stylelint:master. There are also random failing tests due to async callback timeouts.

Here's the tests on stylelint:master running on my machine:

screen shot 2017-02-07 at 22 57 57

Here's the tests on sergesemashko:implement-caching running on my machine:

screen shot 2017-02-07 at 23 05 53

(Note that the failing tests will be different on each run)

The stylelint:master tests run in around ~90 seconds, and the implement-caching tests around ~680 seconds. This is a big jump and I suspect it's related to the CI failures. I haven't taken a close look at the code but hopefully that's enough info for someone to do some investigation.

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 8, 2017

@m-allanson, thanks for checking and pointing out to that. I'll investigate the reason. Definitely don't want to introduce any issues for testing :)

@davidtheclark

@sergesemashko 🥇 It's awesome that you are taking this on, and that you researched it so well. Interesting stuff! It's fun for me to learn new things reviewing this, and to think about some new problems.

Sorry for the delay reviewing: I knew I would need to find a chunk of time to read the code thoroughly and put effort into understanding the details of such a new feature.

I left several questions and comments. Looking forward to your response!

@@ -77,6 +77,20 @@ If `true`, all disable comments (e.g. `/* stylelint-disable block-no-empty */`)
You can use this option to see what your linting results would be like without those exceptions.
## `cache`
Store the info about processed files in order to only operate on the changed ones. The cache is stored in `.stylelintcache` by default. Enabling this option can dramatically improve Stylelint's running time by ensuring that only changed files are linted.

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Nitpick: we have been writing "stylelint" lowercase.

Store the info about processed files in order to only operate on the changed ones. The cache is stored in `.stylelintcache` by default. Enabling this option can dramatically improve Stylelint's running time by ensuring that only changed files are linted.
**Note:** If you run Stylelint with `--cache` and then run Stylelint without `--cache`, the `.stylelintcache` file will be deleted. This is necessary because the results of the lint might change and make `.stylelintcache` invalid. If you want to control when the cache file is deleted, then use `--cache-location` to specify an alternate location for the cache file.

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

This is a little confusing to me: does that mean that if I use --cache-location, my cache file will become outdated when I run without --cache, because it will not be deleted? I'm not sure I'd want this implicit behavior change solely because I specified a cache file location.

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

Actually, cache file will be deleted when --cache-location is not changing between runs with --cache and without. Cache file won't be deleted for the following case:

  1. stylelint *.scss --cache - create cache file in default location
  2. stylelint *.scss --cache-location /tmp - won't delete cache file from default location
  3. stylelint *.scss --cache - reuse cache file from default location

This comment has been minimized.

@davidtheclark

davidtheclark Feb 15, 2017

Contributor

Ok, thanks for clarifying. I think we should remove this sentence from the docs because it represents an edge-case that is fairly confusing. If people ask about it, we could add a note somewhere else, like the FAQ.

Path to the cache location. Can be a file or a directory. If no location is specified, `.stylelintcache` will be used. In that case, the file will be created in the directory where the `stylelint` command is executed.
If a directory is specified, a cache file will be created inside the specified folder. The name of the file will be based on the hash of the current working directory (CWD). e.g.: `.cache_hashOfCWD`

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Why use a hash filename instead of simply adding .stylelintcache to that directory?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

This will allow to reuse a single location for the caches from different projects and still receive the benefits of the cache. I'll add this sentence to the doc.
see eslint/eslint#4255 (comment).

This comment has been minimized.

@davidtheclark

davidtheclark Feb 15, 2017

Contributor

Sounds good!

@@ -0,0 +1 @@
/* This will not cause a CSS syntax error */

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Do you mean this will not cause a linting error? Those "invalid" files also will not cause CSS syntax errors, right?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 16, 2017

Contributor

yep, right, I'll update the wording

mockedFileCache.verify()
})
})
afterEach(() => {

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Can you please put this up by the beforeEach? I find that helps force me to grasp the full context of each test upfront.

expect(fileCache._hashOfConfig).toBe(hashOfConfig)
mockedFileEntryCache.verify()
// restore mocked objects
mockedFileEntryCache.restore()

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

What is the purpose of these two lines? Am I missing an assertion that they relate to?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

agree, line 23 doesn't make sense. Line 24 restores file-entry-cache dependency for further use in case of new tests.

mockedFileEntryCache.restore()
mock.stopAll()
})
it("reconcile() stores hash to descriptor", () => {

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

I'm not sure we need this test. Shouldn't we be able to run tests that determine whether FileCache actually works as expected without caring about the implementation details? This seems like it's just testing implementation details, not the API of the FileCache.

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

FileCache.prototype.reconcile() wraps file-cache-entry.reconcile() to add config hash before saving to every file entry. Just trying to make sure config hash is added on reconcile

getFileDescriptor: getFileDescriptorStub,
},
_hashOfConfig: hashOfConfig,
}

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Instead of creating this mock fileCache object, can't you use stub to stub the real getFileDescriptor method on a real FileCache instance? If so, wouldn't that would also avoid the need to awkwardly use call below?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 16, 2017

Contributor

ok, since I'm going to have more real things in tests, that would be the new way of testing hasFileChanged

expect(hasFileChanged.call(fileCache, invalidFile1)).toBe(true)
expect(hasFileChanged.call(fileCache, invalidFile2)).toBe(true)
})
it("file-cache-entry methods are called", () => {

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Why does this matter for our tests? Aren't these implementation details?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

Just was trying to make sure the constructor is covered by tests. Maybe doesn't make sense when real FileCache is used elsewhere, I'll if it's still needed after update

const fixturesPath = path.join(__dirname, "fixtures")
describe("standalone cache is enabled", () => {
let mockedFileCache

This comment has been minimized.

@davidtheclark

davidtheclark Feb 11, 2017

Contributor

Why are we mocking the file cache instead of testing the real thing? Couldn't we actually use the file cache in our tests, cleaning up after as needed, and then bypass some of the efforts below to test its internals?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 16, 2017

Contributor

Makes sense, I'll change to the real thing

@sergesemashko

I tried to write tests isolating dependencies. Based on the comments I see that it makes sense to test real things in some cases.

  1. I'll update tests.
  2. and investigate performance bottleneck
return getCacheFileForDirectory()
}
return resolvedCacheFile

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

makes sense

expect(fileCache._hashOfConfig).toBe(hashOfConfig)
mockedFileEntryCache.verify()
// restore mocked objects
mockedFileEntryCache.restore()

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

agree, line 23 doesn't make sense. Line 24 restores file-entry-cache dependency for further use in case of new tests.

mockedFileEntryCache.restore()
mock.stopAll()
})
it("reconcile() stores hash to descriptor", () => {

This comment has been minimized.

@sergesemashko

sergesemashko Feb 15, 2017

Contributor

FileCache.prototype.reconcile() wraps file-cache-entry.reconcile() to add config hash before saving to every file entry. Just trying to make sure config hash is added on reconcile

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 15, 2017

Awesome work @sergesemashko!

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 20, 2017

What's in the latest update:

  • All the tests are rewritten to test real dependencies. Actually, tests replicate real use cases now. The only thing I'm not sure about whether AppVeyor allows to do file manipulations in tests, because tests are failing there, when everything is ok on local. @evilebottnawi, @davidtheclark, do you have any insights on it or preferences how to deal with file manipulations in tests?
  • Reflected comments in the code, @davidtheclark thanks a bunch!
  • Reduced the overhead and execution time looks great now.
@davidtheclark

Great! I made a few copy edits directly, hope you don't mind. Left a couple of comments for you, also. I think we're pretty close.

const standalone = require("../standalone")
const hash = require("../utils/hash")
const fixturesPath = path.join(__dirname, "fixtures")
const fsExtra = require("fs-extra")

This comment has been minimized.

@davidtheclark

davidtheclark Feb 23, 2017

Contributor

I'd rather we avoid this new dependency and just use regular fs. For copying you could use https://github.com/sindresorhus/cpy. Additionally, we should be able to avoid sync calls for all of this because Jest has good async/Promise support.

This comment has been minimized.

@sergesemashko

sergesemashko Feb 23, 2017

Contributor

thanks, I'll try to avoid sync calls and minimize dependencies

expect(output.errored).toBe(true)
// Ensure only changed files are linted
const isValidFileLinted = !!output.results.find(file => file.source === validFile)
const isInvalidFileLinted = !!output.results.find(file => file.source === changedFile)

This comment has been minimized.

@davidtheclark

davidtheclark Feb 23, 2017

Contributor

Should that be invalidFile instead of changedFile?

This comment has been minimized.

@sergesemashko

sergesemashko Feb 23, 2017

Contributor

I realized that changedFile is a confusing var name here, it should be something like newFileDest. The idea here is to create new file so standalone should process both cached and uncached files.

// Ensure cache file doesn't contain invalid file
const cachedFiles = fsExtra.readJsonSync(expectedCacheFilePath)
expect(typeof cachedFiles[validFile] === "object").toBe(true)
expect(typeof cachedFiles[changedFile] === "undefined").toBe(true)

This comment has been minimized.

@davidtheclark

davidtheclark Feb 23, 2017

Contributor

Again, wondering if this should be invalidFile.

This comment has been minimized.

@sergesemashko

sergesemashko Feb 23, 2017

Contributor

I'll update the var as per comment

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 23, 2017

@jeddy3 I think we're close on this one. Do you want to give it a review? It's a pretty significant feature, so I want to make sure I'm not the only one who looks it over.

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 23, 2017

@davidtheclark, thanks for the copy edits and feedback.
Maybe you have any ideas why file operations are failing in appveyor build?

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 23, 2017

@sergesemashko I'm afraid not. I try to ignore Appveyor. We have some Windows users in @stylelint/core. One guess: have you used path functions to normalize path separators?

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 23, 2017

@davidtheclark, good idea, I'll try normalized paths in tests.

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 25, 2017

@davidtheclark, Here are the latest updates:

  1. Sync calls are converted to promises, however I decided to leave fs-promise(a promisified fs-extra version) because It required ways to read, remove and copy files using promises. And I though implementing the same functions using just fs would be redundant.
  2. Issue with paths was fixed after I added normalize to paths returned by globby()
@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Feb 28, 2017

@davidtheclark @evilebottnawi @jeddy3, could you please take a look?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Feb 28, 2017

@sergesemashko can you squash commits for easy review?

@sergesemashko sergesemashko force-pushed the sergesemashko:implement-caching branch from bfce140 to 5ceec77 Mar 3, 2017

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

sergesemashko
Tests: cover cache feature implementation (stylelint#2293)
Fix flow annotations (stylelint#2293)

Add test to cover cache implementation (stylelint#2293)

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

sergesemashko
Optimization: Run cache init code only when globby() is used (styleli…
…nt#2293)

- Refactored sync calls into promises.
- Copy edits.

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Mar 19, 2017

@evilebottnawi, thanks for your feedback.

  • What about invalidate cache file if i change configuration?

it's already accounted. Every cached entry has the hash of the config. If config changes all the files will be re-linted because hashes don't match. Checkout this line

  • Why we use multiple cache files? 556168c#diff-25bbf797b85423b576b2d765600b119aR33

This will allow to reuse a single location (let's say /tmp) for the caches from different projects and still receive the benefits of caching.
see eslint/eslint#4255.

Thanks, wasn't aware of this package. Looks like it may be helpful for any other cases and it looks much lighter that fs-promise. Will update soon.

  • I think we should remove all debug function from this commit and do this in separate PR.

Didn't quite get your concerns. Do you see potential issues with introducing debug and want keep discussion in separate PR?
TBH, I'd like keep it here, if possible, as cache profiling rely on using debug and its usages look pretty straightforward.
Separate PR would just mean more work for me and for the reviewers. If you know something that might be affected by adding debug within this PR, please let me know.

@sergesemashko sergesemashko force-pushed the sergesemashko:implement-caching branch 2 times, most recently from 903d7c5 to e7b6ef3 Mar 19, 2017

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 19, 2017

sergesemashko
Issue stylelint#2270 - Cache implementation (stylelint#2293)
Replace fs-promise by cpFile and pify

Optimized pify usage

Add cache flow param annotations (stylelint#2293)

@sergesemashko sergesemashko force-pushed the sergesemashko:implement-caching branch from 02a4feb to bd9654d Mar 20, 2017

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 20, 2017

sergesemashko
Issue stylelint#2270 - Cache implementation (stylelint#2293)
Replace fs-promise by cpFile and pify

Optimized pify usage

Add cache flow param annotations (stylelint#2293)
(cherry picked from commit e7b6ef3)

Fix flow annotation for FileCache
(cherry picked from commit f7a593b)
sergesemashko
Issue #2270 - Cache implementation (#2293)
Replace fs-promise by cpFile and pify

Optimized pify usage

Add cache flow param annotations (#2293)
(cherry picked from commit e7b6ef3)

Fix flow annotation for FileCache
(cherry picked from commit f7a593b)

@sergesemashko sergesemashko force-pushed the sergesemashko:implement-caching branch from 79743b8 to 1dd4951 Mar 20, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 20, 2017

@sergesemashko Thanks for answer.

Didn't quite get your concerns. Do you see potential issues with introducing debug and want keep discussion in separate PR?
TBH, I'd like keep it here, if possible, as cache profiling rely on using debug and its usages look pretty straightforward.
Separate PR would just mean more work for me and for the reviewers. If you know something that might be affected by adding debug within this PR, please let me know.

No i don't see problems, we should add debug for all code or nothing. If this is already done, it will be fine 😄

SGTM for me 🥇

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Mar 20, 2017

@evilebottnawi , I see. Frankly speaking, adding debug for all codebase sounds like a "big deal" for me 😃 I'm not yet that familiar with all the cases where it really should be added.
Maybe, gradually introducing debug to the files as we go would work?
Debug namespaces are already unique per file, e.g. stylelint:standalone, stylelint:file-cache, so it can be easily introduced to other files when necessary.
I know it sounds slippery, but I think it's worth suggesting :) If it still doesn't work, I'll extract debug and will keep the discussion in separate PR.
cc/ @davidtheclark

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 25, 2017

@jeddy3 are you good with merging this?

@jeddy3

jeddy3 approved these changes Mar 26, 2017

@jeddy3 are you good with merging this?

Yeap. This is awesome!

(Sorry about the delay btw, I've only just got around catching up on stylelint after a very busy couple of months)

@sergesemashko Thanks! And welcome to the team :)

@jeddy3 jeddy3 merged commit 43b18fe into stylelint:master Mar 26, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 94.999%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 26, 2017

Added to changelog:

  • Added: cache option to store the info about processed files in order to only operate on the changed ones the next time you run stylelint (#2293).
@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 31, 2017

@sergesemashko I experienced the work of the cache and it's great! Only I noticed one strangeness:
we ignore by default node_modules and bower_components, but if package have .scss (or .css) and i update package, this files fall as if not cached. I think problems in here https://github.com/stylelint/stylelint/blob/master/lib/standalone.js#L144 you don't exclude files from node_modules and bower_components.

My npm script: stylelint '{**/*,*}.scss' --ignore-path .gitignore. I don't know it is bug or not, just information.

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Mar 31, 2017

@evilebottnawi, thanks for the input.
Cache info for specific file is used depending on the output of file-entry-cache that checks if file has changed. And cache is invalidated for all changed files.
Could you please clarify what's your expectation from linting dependency styles?
I may also not clear yet what's the exact issue :)

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 1, 2017

@sergesemashko default command stylelint '{**/*,*}.scss' --ignore-path .gitignore globbing all files also inside in node_modules :(

@olegskl

This comment has been minimized.

olegskl commented Apr 10, 2017

I was wondering why is fileCache initialized on line 105, but prepareReturnValue is called on line 90 inside the return statement? When there are no files (linting code as a string) this throws:

Cannot read property 'reconcile' of undefined

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 10, 2017

@olegskl can you create issue?

@sergesemashko

This comment has been minimized.

Contributor

sergesemashko commented Apr 10, 2017

@olegskl, thanks for reporting this. fileCache is initialized after

if (!files) {
    const absoluteCodeFilename = (codeFilename !== undefined && !path.isAbsolute(codeFilename))
      ? path.join(process.cwd(), codeFilename)
      : codeFilename
    return stylelint._lintSource({
      code,
      codeFilename: absoluteCodeFilename,
    }).then(postcssResult => {
      return stylelint._createStylelintResult(postcssResult)
    }).catch(handleError).then(stylelintResult => {
      return prepareReturnValue([stylelintResult])
    })
  }

because this block is executed when raw styles are passed. So, there is no need to initialize fileCache before this block, because there is no files to cache.

However, looks like you found a bug, which wasn't covered by tests. @olegskl, could you please submit an issue? And I'll take a look at it.

@olegskl

This comment has been minimized.

olegskl commented Apr 11, 2017

@sergesemashko here it is #2492, sorry for the late reply.

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