Skip to content
This repository has been archived by the owner on Sep 9, 2021. It is now read-only.

fix: do not match too many characters when removing sourceMappingURL … #286

Merged
merged 4 commits into from Sep 22, 2020

Conversation

schl3ck
Copy link
Contributor

@schl3ck schl3ck commented Sep 10, 2020

…comment

a sourceMappingURL comment may be created by another loader and this removes it again, but too many
characters are removed, thus uncommenting a different line that contains invalid javascript

Closes #285

This PR contains a:

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

Motivation / Use-Case

See #285

…comment

a sourceMappingURL comment may be created by another loader and this removes it again, but too many
characters are removed, thus uncommenting a different line that contains invalid javascript

Closes #285
@jsf-clabot
Copy link

jsf-clabot commented Sep 10, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #286 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   74.48%   74.48%           
=======================================
  Files           6        6           
  Lines         145      145           
  Branches       52       52           
=======================================
  Hits          108      108           
  Misses         31       31           
  Partials        6        6           
Impacted Files Coverage Δ
src/utils.js 80.55% <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 17f81c1...e793510. Read the comment docs.

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.

Can we add tests?

src/utils.js Outdated
@@ -87,7 +87,7 @@ ${
}

// Matches only the last occurrence of sourceMappingURL
const innerRegex = /\s*[#@]\s*sourceMappingURL\s*=\s*([^\s'"]*)\s*/;
const innerRegex = /\s*[#@]\s*sourceMappingURL\s*=\s*((?:[^\s'"\\]|\\[^n])*(?:\\n)?)\s*/;
Copy link
Member

Choose a reason for hiding this comment

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

^n? Looks it is invalid

Copy link
Contributor Author

@schl3ck schl3ck Sep 11, 2020

Choose a reason for hiding this comment

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

The idea was to match everything except all whitespace, quotes and \n literally (not the line feed character), but not a backslash on its own. It works like a negative lookahead but consuming characters. Another possible solution would be .*?(?=[\s'"]|\\n)(?:\\n)?, which actually looks a lot more readable (instead of (?:[^\s'"\\]|\\[^n])*(?:\\n)?)

And I've just noticed that with the current regex the match wouldn't stop when it encounters a whitespace or quote after a backslash, but it would have done before. Also the test will fail, if it is in a multiline comment. I will work on a fix. Thanks for not letting this slip past!

…lashes when it shouldn't

The regex is also much more readable
Fix failing test when `sourceURL=webpack-internal:///...` is inside a multiline comment
@schl3ck
Copy link
Contributor Author

schl3ck commented Sep 16, 2020

I don't know why the lint job fails. I've tested it at the v3.0.2 tag and it faild there too just so you know.

@alexander-akait
Copy link
Member

@schl3ck All fine, don't worry, I will look at this in near future

@alexander-akait alexander-akait merged commit 0d4624c into webpack-contrib:master Sep 22, 2020
TheLD6978 pushed a commit to TheLD6978/worker-loader that referenced this pull request Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax error: unexpected token: ':' when setting option inline to no-fallback
3 participants