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

Add support for --cache #132

Open
gunta opened this issue Jan 20, 2016 · 8 comments
Open

Add support for --cache #132

gunta opened this issue Jan 20, 2016 · 8 comments

Comments

@gunta
Copy link

gunta commented Jan 20, 2016

Hi! This has been discussed before: #99

However there are 3 problems with that solution:

  • Cache is not persisted to disc
  • It involves effort on the user, where one would expect just to set up a simple cache: true variable
  • Eslint command line already does it in a well defined .eslintcache

Thank you!

@adametry
Copy link
Owner

While ESLint's CLI has the responsibility of finding and reading files for linting, gulp-eslint relies on the Gulp stream to manage file sources. If a file should not be linted, it should not be passed to gulp-eslint in the stream.

The cached-lint example demonstrates how caching could be managed for a gulp stream; though, is not intended to be a direct implementation of ESLint's CLI cache option, nor is it the only way to avoid needless file linting.

Fortunately, it is possible to persist a file/content cache to disc in a gulp stream (see gulp-cache-money or gulp-file-cache). However, I'm not aware a gulp plugin based on file-entry-cache (the cache package used by ESLint). If another file-based cache plugin doesn't work in its place, perhaps creating a gulp plugin based on file-entry-cache would be useful with gulp-eslint as well as many other gulp plugins.

Though cache:true would be an easy option to configure, its simplicity is due to assumptions that must be made about the environment and use case. ESLint's CLI acts as an independent tool, run directly against the file system. Because of this, a simple --cache option for a file-based cache is a stable and practical solution. On the other hand, Gulp's virtual files may come from a range of sources and may be composed of a series of tools/plugins. For a plugin, it is uncertain whether a virtual file's origin (if there is one) will match it's destination (if there is one) or how the content may be transformed along the way. However, caching can be considered when composing a gulp task, where the expectations are certain.

@gunta
Copy link
Author

gunta commented Jan 21, 2016

I agree with that, and it all makes perfect sense from a gulp/implementation point of view.

From a user point of view, however, you are to expect a solution since if you come from straight eslint or grunt-eslint; it looks as it may be gulp-eslint responsibility.

To mitigate this we have the following choices:

  1. Create a new gulp plugin for file-entry-cache, and add a sample in gulp-eslint.
  2. Create a different gulp-eslint implementation (either as a separate gulp plugin, or as a configuration parameter inside gulp-eslint), where it uses an executeOnFiles implementation,
    thus being able to use the eslint cache native implementation.

In short: its either about being more on the gulp native way, or eslint native way of doing things.

@gunta
Copy link
Author

gunta commented Jan 21, 2016

Note: I'm ok with both of the solutions, however if we look at the path that eslint 2.0 is taking, the 2nd. option might make more sense.

For example:
eslint/eslint#3561
Breaking: Ignore bower_components by default

This would be only feasible with an executeOnFiles implementation, and having different behaviours between eslint and gulp-eslint is definitely raise issues for newcomers ;)

@adametry
Copy link
Owner

Grunt is a file-based task runner (each task acts upon a file query and works directly with files). Like with ESLint CLI, it makes sense to use a file-based cache. For the reasons explained previously, Gulp manages files in a different way.

Gulp-eslint is a plugin to lint files passing through a gulp stream, not a way of using Gulp as a trigger for running the ESLint CLI against a separate file system. If you want it to just work like ESLint, it makes more sense to use ESLint's CLI or, if needed, a gulp plugin that is designed for triggering CLI (e.g., gulp-shell, gulp-exec).

Using executeOnFiles would mean ignoring the streaming (and possibly transformed) source of the virtual files from the gulp stream and instead reading content from the file system directly (if there's even a file path available). This is a fundamental conflict with using a virtual file stream.

ESLint already ignores any file path (even when using executeOnText) that starts with "node_modules", which is a problem for gulp-eslint since it can be passed virtual files from node_modules (whether by accidental or intentional). It appears that, the "bower_components" change will be just as problematic. However, this is immaterial to the discussion about a "cache" option.

@gunta
Copy link
Author

gunta commented Jan 22, 2016

From a gulp's way gulp-eslint point of view then, the following solution would make sense, right?

  1. Create a new gulp plugin for file-entry-cache, and add a sample in gulp-eslint.

@shinnn
Copy link
Collaborator

shinnn commented Jan 25, 2016

Create a new gulp plugin for file-entry-cache

No need to create a new one. Various file cache plugins already exist.

@evtk
Copy link

evtk commented Oct 13, 2016

Hey all, been trying to get eslint work with a file based cached. I want to run my lint task on manual start (no watch task, so no in memory cache possible). If been reading the topics around here on the subject of eslint cache. Based on the examples @adametry has provided, I think something like below should work. I'm using gulp-asset-cache for this matter.

Option 1

function lint() {
  return gulp.src(config.filesPatterns.appNoHtml2Js)
    .pipe($.assetCache.filter())
    .pipe($.using())
    .pipe($.eslint({
      configFile: config.folders.root + '.eslintrc',
      fix: config.lint.autofix
    }))
    .pipe($.eslint.format(eslintReporter, function (results) {
      fs.writeFileSync(path.join(config.folders.report, config.lint.reportFile), results);
    }))
    .pipe($.if(isNotLinty, $.assetCache.cache()))
    .pipe($.if(isFixed, gulp.dest(config.folders.app)));
}

function isNotLinty(file) {
     return file.eslint != null && (file.eslint.errorCount=== 0);
}

Option 2

function lint() {
  return gulp.src(config.filesPatterns.appNoHtml2Js)
    .pipe($.assetCache.filter())
    .pipe($.using())
    .pipe($.eslint({
      configFile: config.folders.root + '.eslintrc',
      fix: config.lint.autofix
    }))
    .pipe($.eslint.format(eslintReporter, function (results) {
      fs.writeFileSync(path.join(config.folders.report, config.lint.reportFile), results);
    }))
    .pipe($.eslint.result(function(result) {
        if (result.errorCount === 0) {
            $.assetCache.cache();
        }
    }))
    .pipe($.if(isFixed, gulp.dest(config.folders.app)));
}

Now... it's a shame, but i can't get both to work. Option 1 has troubles with the gulp-if. Although the condition is cleary true or false based on the errorCount, it seems like the condition is always true, because all files (even the ones with errors) are added to the cache. Option 2 seems to never call the cache function, or not in the right way, because the cache is not created.

Any suggestions? Thanks!!!

@acierto
Copy link

acierto commented Dec 26, 2018

You can find here a solution how to overcome that issue: https://dzone.com/articles/how-to-improve-performance-of-eslint-in-gulp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants