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: lint modules that are cached with webpack's filesystem cache #197

Merged
merged 4 commits into from Feb 3, 2023

Conversation

facugaich
Copy link
Contributor

This PR contains a:

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

Motivation / Use-Case

If webpack is setup with cache type filesystem, the succeedModule hook is not called for cached modules and no linting is run for them. Tap the stillValidModule hook to lint cached modules.

Note regarding testing, I tried several strategies but the only one that I managed to get working was to create two different compilers and have the second one pickup the cache left by the first one.

Fixes #130

Breaking Changes

Additional Info

If webpack is setup with cache type `filesystem`, the `succeedModule`
hook is not called for cached modules and no linting is run for them.
Tap the `stillValidModule` hook to lint cached modules.

Fixes webpack-contrib#130
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 28, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: facugaich / name: Facundo Gaich (cfb2ec8)

@alexander-akait
Copy link
Member

/cc @ricardogobbosouza hello, can you review?

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (0cd8d4f) compared to base (ebce9be).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #197   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          280       283    +3     
  Branches        78        78           
=========================================
+ Hits           280       283    +3     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@instagibb
Copy link

Is this gonna be reviewed/merged anytime soon? Really keen to see this in a release

@ricardogobbosouza
Copy link
Collaborator

Sorry for the delay, my considerations:

  1. The lint will run again on the cached files, impairs the performance.
  2. The plugin should retrieve errors and warnings from cached files as @alexander-akait mentioned here ESLintPlugin not working consistently with cache.type = 'filesystem' #130 (comment)
  3. I'm going to merge this PR because it solves this problem, but we need to deal with the performance

@ricardogobbosouza ricardogobbosouza merged commit 92f25ec into webpack-contrib:master Feb 3, 2023
@facugaich
Copy link
Contributor Author

@ricardogobbosouza Thank you!

Re: performance, could we use ESLint's own cache option for that? It seems like it would work for the non-threaded case at least, and you've mentioned that the threaded impl should be deprecated anyways in #143

@yoyo837
Copy link

yoyo837 commented Feb 7, 2023

This changes caused Hot Module Replacement build to be very slow after save files.

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.

ESLintPlugin not working consistently with cache.type = 'filesystem'
5 participants