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

Fix #43, #44, #45 #47

Merged
merged 5 commits into from
Nov 30, 2020
Merged

Fix #43, #44, #45 #47

merged 5 commits into from
Nov 30, 2020

Conversation

jsg2021
Copy link
Contributor

@jsg2021 jsg2021 commented Nov 20, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fix #43, #44, and #45

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #47 (034a29a) into master (46fc07a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #47   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          200       219   +19     
  Branches        55        59    +4     
=========================================
+ Hits           200       219   +19     
Impacted Files Coverage Δ
src/options.js 100.00% <ø> (ø)
src/getESLint.js 100.00% <100.00%> (ø)
src/linter.js 100.00% <100.00%> (ø)
src/worker.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46fc07a...034a29a. Read the comment docs.

@jsg2021 jsg2021 changed the title WIP: Fix #44 Fix #44 Nov 20, 2020
@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 23, 2020

@evilebottnawi @ricardogobbosouza this should fix #44 and #45

@ricardogobbosouza
Copy link
Collaborator

/cc @evilebottnawi

@@ -51,6 +58,9 @@ export default function linter(options, compilation) {
* @param {string | string[]} files
*/
function lint(files) {
for (const file of asList(files)) {
delete crossRunResultStorage[file];
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need manually delete it? When you remove something from WeakMap often means that you are using it out of place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats not the WeakMap. The WeakMap keeps a compiler instance to (file=>lintresult map) mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we lint a file and it does not contain errors, we don't want the old result just in case we do not get a result back from the linter.

Copy link
Member

Choose a reason for hiding this comment

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

Why we need to keep old result?

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's the point of #44 ... we need to keep reporting results until they are resolved. In watch mode if the user edits a file we don't want to discard all the other file results and only show the one file's results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In each run of the compiler, we only get the results of the files we lint... in watch mode, that may only be one file. But this should be cumulative not just the last run.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explanation

@jsg2021 jsg2021 changed the title Fix #44 Fix #43, #44, #45 Nov 30, 2020
@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 30, 2020

I added a fix for #43 as well.

@alexander-akait
Copy link
Member

/cc @ricardogobbosouza I think we can merge

@ricardogobbosouza ricardogobbosouza merged commit 4b8d4de into webpack-contrib:master Nov 30, 2020
@jsg2021 jsg2021 deleted the fix-44 branch November 30, 2020 16:59
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

Successfully merging this pull request may close these issues.

Enabling fix doesn't seem to update source files
3 participants