WebMemex / webmemex-extension

📇 Your digital memory extension, as a browser extension
https://webmemex.org
Other
208 stars 45 forks source link

Add ESLint watch (fix #72) #87

Closed reficul31 closed 7 years ago

reficul31 commented 7 years ago

Refs DO NOT MERGE YET: I would like to resolve all the sub tasks on the issue list for this issue in this PR itself. I still need to run the linter in build and test. I was thinking of simply cascading the eslint command with the gulp watch and the test. Any advice @Treora ?

Here I have added a gulp task called lint-watch which lints the files. Also I have changed the name of watch to build-watch and added another gulp task called watch which basically combine both lint-watch and build-watch. This will help us implement the watch over every file and lint it as soon as any change takes place.

NOTE: For some reason there is a lot of delay in running the lint. On my machine I was constantly getting a minute or 50s to lint all the files after the change takes place, and theses were when I waited after every changes. Its worse for consecutive changes taking place very quickly.

EDIT: Good to see Travis in action. :)

Treora commented 7 years ago

If you use gulp.watch this way, it will rerun the whole linter on every change, which would be a waste of computation. You want to run the linter only on the files that changed. I am not an expert at gulp, but I would hope a list of changed files can be obtained from it. Also, there may be tools that watch file changes and run the linter on them, so gulp only has to start that process once, just like watchify. Is the eslint-watch you found not suitable for this?

Treora commented 7 years ago

Also I hope you don't mind me changing the titles of you PRs; it's nice you mention the issue number but I always end up confusing the PR with the issue itself.

reficul31 commented 7 years ago

Is the eslint-watch you found not suitable for this?

As far as I know from reading the docs, eslint watch is a wrapper around the eslint command line tool with eslint watch allowing certain functionalities such as --watch. Since we decided to use the gulp way I couldn't think of a suitable implementation with the eslint watch. So basically eslint watch is a CLI with no gulp support, therefore I thought of using gulp-eslint.

Also I hope you don't mind me changing the titles of you PRs

No problem at all. I will keep this in mind while making my next PR.

So, currently I am trying to use the

gulp.task('lint-watch', () => {
    gulp.watch(['**/*.js', '**/*.jsx'])
    .on('change', (file) => {
        return gulp.src(file.path)
        .pipe(eslint())
        .pipe(eslint.format())
    })
})

But it has one issue. No linting takes place when the command is first called and only the files changed when the watch command is run are linted. I am trying to find a way around this but every approach seems to slow down the watch process considerably. I'll keep looking. If i can't find a way I'll send a PR. But the files are linted as soon as they are changed so that is good and linting speed is increased.

edit: the first build is slow. It takes about a minute to 50s on machine. Also the first lint is also slow and took a mind boggling 2.52 mins on my machine. Subsequent lints are quick enough though.

Treora commented 7 years ago

@reficul31: did you happen to find solutions, or have hints to share if others (e.g. myself) would try solve this?

reficul31 commented 7 years ago

@Treora I was looking through some of the plugins we could possibly add to this to maybe serialize the tasks such as run-sequence . I am really sorry I cannot have a look at this right now since I am in the middle of my university exams. I'll try and explain the problem as clearly I can so someone can take over or suggest some solution.

The PROBLEM When we run sudo make watch the process is extremely slow. I think this is due to the fact that the process launches 3 threads that try to complete the tasks simultaneously but if we have a look at the code, we want sudo gulp lint-watch and sudo gulp lint to run in succession(lint before the lint-watch).This is because if we just run the lint watch command the files already changed are not linted. The files changed thereafter(after running the command) are watched and linted. So we want the command to do an initial lint of the whole src and then start the watch. This is a very slow process as it takes about 50s to bundle all the files(build-watch), 9s to start the lint-watch and a very slow 2.3m to end the lint.

edit: also in the updated PR I have integrated the lint with build and build-prod tasks respectively. I hope this was the implementation @Treora had in mind for the task

Run and print warnings in make build.

Also, should we edit the README to show how to use the linter?

reficul31 commented 7 years ago

I have added run-sequence against my will just for the fact that it speeds up the process. For now I think this will work. Please tell me if any more changes are required. I will continue looking for better methods of doing this.

Treora commented 7 years ago

It was so slow because you took all javascript files, not just the source. I took the first commit of the two, reduced slowness by only linting src/, merged it to master. Thanks!

I did not bother about also watching for css-lint. Not so important.