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: strip resource query #58

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

privatenumber
Copy link
Contributor

@privatenumber privatenumber commented Dec 22, 2020

This PR contains a:

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

Motivation / Use-Case

Prior to this PR, eslint-webpack-plugin was not stripping Webpack path queries in resource paths and was failing to resolve files with resource queries in its path (eg. "virtual files" created by Webpack loaders). This is a regression introduced in v2.2.0.

For example, mdvue-loader loads Markdown files as Vue components and also renders Vue code-blocks in the markdown file. To accomplish this, it imports the specific code-block as a virtual module via a query path like this:

/project/README.md?vue&type=script&lang=js&mdvue-file=Demo.vue

Given a ESLintPlugin configuration like this, the .vue at the end of the query makes micromatch detect it as a file:

new ESLintPlugin({
	files: '**/*.{vue,js,md}',
	emitWarning: true,
})

Breaking Changes

Additional Info

  • The fix strips resource queries using url.parse.
  • url.parse is actually deprecated, but decided to still use it as new URL might not be available in older versions of Node.js (globalized in v10). new URL also doesn't support relative paths.
  • It might be fine to just do module.resource.split('?')[0] as well but unsure how well that scales. Seems like macOS allows ? in the filename, and that seems to break url.parse too.

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #58 (8ed4f53) into master (2e6c8eb) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #58   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          239       239           
  Branches        65        65           
=========================================
  Hits           239       239           
Impacted Files Coverage Δ
src/index.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 2e6c8eb...8ed4f53. Read the comment docs.

src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @ricardogobbosouza I think we need to fix and look at new issues

@decademoon
Copy link

This causes the same module file loaded with different resource queries to be linted multiple times. See #70

@privatenumber
Copy link
Contributor Author

@decademoon

I wouldn't say this caused the bug, but rather surfaced it.

Prior to this PR, this plugin was reading the query as if the file extension was in there. That shouldn't be happening.

I think the solution you propose--tracking linted files to catch duplicates--is a good idea.

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.

None yet

4 participants