Skip to content
This repository was archived by the owner on Sep 28, 2020. It is now read-only.

Ignore cache when eslint rules have changed #126

Conversation

peternewnham
Copy link
Contributor

@peternewnham peternewnham commented Nov 23, 2016

This is an attempt to fix #124

I've tried the following approach to tackle the problem:

  • Get a lint rules object for each CLIEngine instance (using CLIEngine.getConfigForFile)
    • This is taken from the rules of the first file to be processed for each engine (I assume every file processed by the same engine will share the same lint rules)
  • Hash the rules object
  • Include the hashed rule in the cache against each file so it can also be compared when deciding whether to use the cache

This seems to function well in my testing and changing the lint rules ignores previously cached results. I did try using a rules hash per file instead of per engine but it was not performant at all - each time it fetches the rules for a file takes between 70-90ms which adds significant overhead for a large project (I saw 15 seconds added to my build containing 350 files when using this method whereas my submitted solution has minimal time increases)

The only caveat I can think of is that if you change the eslint rules whilst the webpack build is in watch mode and then change a file, the rules hash will not be updated in the cache so when the build is stopped and started again the file will not be cached because it will have a new rules hash to compare again but I don't think this is anything to be concerned about.

I still have tests to add and will do so if you are happy with this solution. I have added a test for the cache option which also covers rule caching.

test/cache.js Outdated
// delete the require cache for eslint-loader otherwise any previously run
// tests will have initialised the cache as false and prevent this test
// from creating the cache file
delete require.cache[require.resolve("../index.js")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is the best solution but all tests share the same instance of eslint-loader so without clearing the require cache, the cache option get initialised as false in previous tests and then it is impossible set it again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use another test runner that will use different processes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea how we do that? According to this issue tape has no built in way to do it so the options appear to be:

  • create a new test command just for this new file and ignore it in the main test command
    • doesn't feel to me like a "good" solution
    • not scalable if there are other tests that need to be run in isolation
  • change the test command to something like find ./test -maxdepth 1 -name '*.js' | xargs -L 1 tape {} which would run all files in a separate tape process
    • slow
    • more work required to collate all results together
    • files with multiple tests in them would still share an eslint-loader instance
    • won't work on non *nix environments
  • delete the require cache
    • additional manual setup needed for every test due to lack of "beforeEach" hook
    • not sure if this is considered good practice

With multiple engines being introduced it seems like a good idea if all tests are run in isolation now to prevent engines being left over and reused (although that is beyond the scope of this PR)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use jest or ava to get separate process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds sensible but out of scope for this PR. I'll create a new issue to gather thoughts on a restructure to testing and then this PR can wait for that.

@peternewnham peternewnham force-pushed the ignore_cache_rule_change branch from ff7f6aa to cc7980c Compare November 28, 2016 00:10
@MoOx
Copy link
Contributor

MoOx commented Feb 1, 2017

I guess you can rebase on master and now benefit of the new test architecture :)

@peternewnham
Copy link
Contributor Author

I appear to have messed up rebasing somehow so have created a new PR for this

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache persists when eslint rules have changed
6 participants