-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: ESLint class migration #11
feat: ESLint class migration #11
Conversation
Refactor the linter method to run asynchronously because many methods have been changed to run asynchronously in the new ESLint class. Preperation for migration to the ESLint class, see webpack-contrib#10.
Refactor to asynchronous formatter loading because the new ESLint class loads formatters asynchronously. Move formatter loading to the linter file for access to the ESLint instance because the new ESLint loadFormatter method is not static. Refactor formatter loading to use a CLIEngine instance. Refactor the linting function to load the formatters. Preperation for migration to the ESLint class, see webpack-contrib#10.
Refactor to filter out loader options before passing the options to ESLint because the new ESLint class throws when unknown options are passed to it. Add method for filtering out loader options and unit tests for it. Rename the getOptions file options because it now contains option utilities. It was possible to have getESLintOptions as a private function in getCLIEngine, but having it exported helps testing. There is no way to query the ESLint class for the options it received so there is no way to test it through the getCLIEngine/ESLint method. Part of the migration to the new ESLint class, see webpack-contrib#10.
Refactor to simplify and remove unnecessary operations. After the formatter loading refactor the getCLIEngine is no longer used without starting a CLIEngine. Remove no longer used options parameter reassignments. Remove unused eslintPath property from the return object. Preperation for migration to the ESLint class, see webpack-contrib#10.
Refactor the custom formatter tests to use a mock. Refactor to test the error output to make sure the selected formatter is being used. The test used to always pass because formatter loading fails silently and returns the default formatter. Add a mock formatter. Remove eslint-friendly-formatter that was only used for the test. This refactor helps decouple the tests from the results format. Making the migration to the new ESLint class easier. Preperation for migration to the ESLint class, see webpack-contrib#10.
Update ESLint development dependency to version 7 to prepare for migration to the new ESLint class that was introduced with it. Part of the migration to the new ESLint class, see webpack-contrib#10.
Refactor the ESLint mock to imitate the new ESLint class. Refactor to simplify mock formatter loading. Refactor ESLint options because the new API introduces a new options format. The tests do not pass yet as the plugin has not been refactored. Part of the migration to the new ESLint class, see webpack-contrib#10.
Refactor linter to the new ESLint class. Refactor the getCLIEngine method to getESLint. Refactor LintDirtyModulesPlugin to remove use of resolveFileGlobPatterns which is not available in the new API. File paths should work fine as globs and do not need to be converted. Completing the migration to the new ESLint class, see webpack-contrib#10.
Update plugin ESLint peer dependency to remove version 6 support. Update the readme to reflect the changes to the plugin. Add warning that the plugin used to use CLIEngine in version 1. Finishing details of the ESLint class migration. Closes webpack-contrib#10.
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 154 155 +1
Branches 37 41 +4
=========================================
+ Hits 154 155 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good @LKummer
Thanks for PR
Just one thing...
src/LintDirtyModulesPlugin.js
Outdated
.resolveFileGlobPatterns(dirtyOptions.files) | ||
.join('|') | ||
.replace(/\\/g, '/'); | ||
const glob = dirtyOptions.files.join('|').replace(/\\/g, '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the links, I will investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick update as I have been silent all week. Thank you for pointing out the issue.
It is indeed causing an issue when watching folder paths with the lintDirtyModulesOnly
option enabled.
I have added tests and reimplemented the functionality of resolveFileGlobPatterns
. I am still refactoring and rebasing, it should be ready for review in a day or two.
Refactor to remove an unnecessary callback from the linter function. It is unnecessary because linter is an async function returning a promise so it can be used directly with a promise tap. Preparation for requested change in webpack-contrib#11.
Refactor LintDirtyModulesPlugin to only handle dirty file filtering. The refactor allows for easier testing without the need for mocks. Rename LintDirtyModulesPlugin to DirtyFileWatcher to make the name more descriptive of the new functionality. Refactor the tests for the simplified DirtyFileWatcher. Refactor the tests to use absolute paths because Webpack passes absolute paths in the compiler.fileTimestamps map. Refactor to only test system specific paths. The functionality of the requested change (webpack-contrib#11) requires using real paths for some tests which causes issues with testing both Windows and Unix paths on the same machine. As the CI runs on Windows, Mac and Linux paths will be tested on all platforms. Preparation for requested change in webpack-contrib#11.
Add failing tests that ensure dirty file watching is matching patterns from the files options exactly like ESLint. Add folder fixture because handling folder paths requires checking existence of folders. Preparation for requested change in webpack-contrib#11.
Fix issue with folder patterns not working with the lintDirtyModulesOnly option. The issue was caused by the removal of resolveFileGlobPatterns which resolved folder patterns into globs matching the correct file extensions. Resolve requested change in webpack-contrib#11.
Refactor LintDirtyModulesPlugin to only handle dirty file filtering. The refactor allows for easier testing without the need for mocks. Rename LintDirtyModulesPlugin to DirtyFileWatcher to make the name more descriptive of the new functionality. Refactor the tests for the simplified DirtyFileWatcher. Refactor the tests to use absolute paths because Webpack passes absolute paths in the compiler.fileTimestamps map. Refactor to only test system specific paths. The functionality of the requested change requires using real paths for some tests which causes issues with testing both Windows and Unix paths on the same machine. As the CI runs on Windows, Mac and Linux paths will be tested on all platforms. Preparation for requested change in webpack-contrib#11.
Add failing tests that ensure dirty file watching is matching patterns from the files options exactly like ESLint. Add folder fixture because handling folder paths requires checking existence of folders. Preparation for requested change in webpack-contrib#11.
Fix issue with folder patterns not working with the lintDirtyModulesOnly option. The issue was caused by the removal of resolveFileGlobPatterns which resolved folder patterns into globs matching the correct file extensions. Resolve requested change in webpack-contrib#11.
Refactor remaining callbacks to async functions to make them consistent with the rest of the taps that have been refactored.
Refactor the linter function to register hooks with the same plugin object that is used to register the run and watchRun hooks.
Refactor to parse patterns only on startup instead of parsing them to globs on each run. In watch mode Webpack does not watch it's own configuration so the plugin configuration does not change when Webpack is running. Because the file patterns and extensions do not change there is no need to process them into globs separately for each run.
Add more information about the files option to the readme.
b5a40ba
to
80b82e6
Compare
I have performed the requested change, as well as some more refactoring and a small readme addition. It is ready for review. (sorry for the force push, I missed a commit message warning) |
/cc @ricardogobbosouza friendly ping |
Thanks @LKummer |
Thank you @ricardogobbosouza for merging. Thank you very much @evilebottnawi for the ping. |
This PR contains a:
Motivation / Use-Case
The CLIEngine class has been deprecated.
This PR refactors the plugin to the new ESLint class.
Closes #10.
Breaking Changes
For more information on the new ESLint options, see the ESLint documentation.
When using deprecated options ESLint will error and suggest how to change the options to the new format.
Additional Info
Changes introduced in the new ESLint API that affect the plugin:
options
has changed and is incompatible withCLIEngine
options
.options
is validated to not contain extra properties.isPathIgnored()
is now asynchronous.resolveFileGlobPatterns()
has been removed.format()
method.The commits form a list of everything that I have refactored with more detailed information.
I have also updated the peer dependency and the readme.
Please mention if you find anything I have missed.