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 files and create a global cache #79

Closed
wants to merge 3 commits into from

Conversation

ricardogobbosouza
Copy link
Collaborator

@ricardogobbosouza ricardogobbosouza commented Feb 5, 2021

This PR contains a:

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

Motivation / Use-Case

Linting the necessary files and create a more intelligent cache of the result

Resolves #75 , #74

Breaking Changes

Additional Info

Thanks @watjurk

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #79 (a696151) into master (f6d26b7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #79   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          250       262   +12     
  Branches        70        74    +4     
=========================================
+ Hits           250       262   +12     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/linter.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 f6d26b7...a696151. Read the comment docs.

@ricardogobbosouza
Copy link
Collaborator Author

/cc @alexander-akait

Can use the invalid hook to invalidate cache?
Apparently it worked well

WDTY?

@alexander-akait
Copy link
Member

invalid for watching, better don't use this hook, it will have same bad side effects with plugins and watching/serve

@ricardogobbosouza
Copy link
Collaborator Author

Can we keep it that way in v2 and fix it in v3? If not, we will have performance problems!

Or do you have any idea how to do this?

@alexander-akait
Copy link
Member

I need to look at this issue, I would prefer to return to it in 3 versions, is it not critical now?

@watjurk
Copy link

watjurk commented Feb 6, 2021

Unfortunately I think this is critical.

it will have same bad side effects with plugins and watching/serve

what side effects ?

@alexander-akait
Copy link
Member

Because some projects use manually invalidate for serve, need investigate how it can break this plugin

@watjurk
Copy link

watjurk commented Feb 12, 2021

@alexander-akait could you provide more details on how this can break this plugin, I'll be happy to investigate.

To resolve issue #83 i would suggest using both compilation.hooks.finishModules and compilation.hooks.succeedModule in following manner:

const seen = new Set()

compilation.hooks.finishModules.tap(ESLINT_PLUGIN, (modules) => {
  /** @type {string[]} */
  const files = [];

  // @ts-ignore
  for (const { resource } of modules) {
    if (resource) {
      const [file] = resource.split('?');

      if (
        file &&
        !seen.has(file) &&
        isMatch(file, wanted) &&
        !isMatch(file, exclude)
      ) {
        seen.add(file)
        files.push(file);
      }
    }
  }

  if (files.length > 0) {
    lint(files);
  }
});

compilation.hooks.succeedModule.tap(ESLINT_PLUGIN, (module) => {
  const { resource } = module
  const [file] = resource.split('?'); //? maybe move this to function
  if (
    file &&
    !seen.has(file) &&
    isMatch(file, wanted) &&
    !isMatch(file, exclude)
  ) {
    seen.add(file)
    lint(file)
  }
})

"Why don't use only compilation.hooks.succeedModule ?"
Because then (in watch mode) we would only lint files that have changed.
"Yes but then we could use crossRunResultStorage."
Then we would again face issue #74.

@ricardogobbosouza ricardogobbosouza mentioned this pull request Feb 12, 2021
6 tasks
@@ -39,8 +38,6 @@ export default function linter(key, options, compilation) {
/** @type {Promise<LintResult[]>[]} */
const rawResults = [];

const crossRunResultStorage = getResultStorage(compilation);
Copy link
Contributor

Choose a reason for hiding this comment

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

This cache held all results that had been computed... invalidations were supposed to be deleted from this and re-run.. so we don't recompute results for files that did not change... but still show them if they had issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related #83

@jsg2021
Copy link
Contributor

jsg2021 commented Feb 19, 2021

This feels like reinventing the results storage that was removed when all that was needed was to remove results for invalidated entries.

@jsg2021
Copy link
Contributor

jsg2021 commented Feb 19, 2021

"Yes but then we could use crossRunResultStorage."
Then we would again face issue #74.

Just delete entries from it on invalidation events.

@watjurk
Copy link

watjurk commented Feb 19, 2021

@ricardogobbosouza can we get #79 (comment) into fix-lint-and-cache branch ?
@alexander-akait could you provide some details on how compiler.invalid could break this PR ?

@ricardogobbosouza ricardogobbosouza deleted the fix-lint-and-cache branch June 15, 2021 18:41
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.

All modules are linted in watch mode after changing only a single file
4 participants