Bugfix/invalid sourcemaps whitespace by connorjclark · Pull Request #41 · webpack/webpack-sources

connorjclark · 2018-09-17T16:45:46Z

OriginalSource created a source map that failed validation (sourcemap-validator). The pos variable tracks the column number for a particular line, but doesn't get incremented when the current token is all whitespace. I believe that only happened for leading whitespace, as all (but possibly the last) token returned by _splitCode would contain some non-whitespace character. The effect was off-by-some-columns errors in the mappings, which manifested in the bootstrap template for webpack (it has leading whitespace).

To create a test, I had to expand the interface for ReplaceSource.replace to get names populated in the source map.

…ains leading whitespace

alexander-akait · 2018-09-17T16:52:06Z

package.json

    "js-beautify": "^1.5.10",
    "mocha": "^3.4.2",
-    "should": "^11.2.1"
+    "should": "^11.2.1",


Why we need should here?

Can you elaborate? The tests use should.

@hoten oh, sorry my mistake, all good 👍

connorjclark · 2018-09-17T16:58:26Z

lib/ReplaceSource.js

 	}

-	replace(start, end, newValue) {
+	replace(start, end, newValue, oldValue = undefined) {


I suppose older versions of Node don't support default values. I'll replace this code. Should be fine to just remove it (undefined is the default default ;) ), but I wanted to be explicit.

alexander-akait · 2018-09-17T17:05:42Z

@hoten CI test failed, need fix

connorjclark · 2018-09-18T18:04:56Z

Pushed fixes

codecov · 2018-09-18T19:13:00Z

Codecov Report

Merging #41 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   75.04%   75.19%   +0.14%     
==========================================
  Files          11       11              
  Lines         505      508       +3     
  Branches       83       83              
==========================================
+ Hits          379      382       +3     
  Misses        126      126

Impacted Files	Coverage Δ
lib/OriginalSource.js	`96.61% <100%> (+0.11%)`	⬆️
lib/ReplaceSource.js	`95.87% <100%> (+0.02%)`	⬆️

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 30d2a80...72a3594. Read the comment docs.

codecov · 2018-09-18T20:49:18Z

Codecov Report

Merging #41 into master will increase coverage by 0.48%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   75.04%   75.53%   +0.48%     
==========================================
  Files          11       11              
  Lines         505      515      +10     
  Branches       83       84       +1     
==========================================
+ Hits          379      389      +10     
  Misses        126      126

Impacted Files	Coverage Δ
lib/OriginalSource.js	`96.61% <100%> (+0.11%)`	⬆️
lib/ReplaceSource.js	`96.01% <100%> (+0.16%)`	⬆️

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 30d2a80...8de1165. Read the comment docs.

michael-ciniawsky · 2018-09-19T17:26:38Z

lib/ReplaceSource.js

 	}

-	replace(start, end, newValue) {
+	replace(start, end, newValue, oldValue) {


newValue => replacer || replacement oldValue => replacee || replaced

(Nitpick)

@michael-ciniawsky not related to this PR, here bug fix, not refactoring

add name to insert

This handles the case when identifiers are renamed twice

sokra · 2018-09-20T08:40:36Z

Thanks

dan-codes-16 · 2018-10-22T14:10:33Z

@hoten did this PR fix the original issue? webpack@4.20.0 uses webpack-sources@1.3.0, but the issue is still reproducible in https://github.com/kenhoff/webpack-sourcemap-testing when webpack version is updated.

connorjclark · 2018-10-22T17:20:10Z

Yeah, I'm still seeing the same issue.

Side note, your test case isn't as intended. You've locked webpack to 4.12.2, and may be using the globally installed version for compilation (prefer npx webpack over webpack).

dan-codes-16 · 2018-10-22T17:49:23Z

@hoten just in case I've forked the repo and updated webpack version to the latest and changed the scripts https://github.com/adaniliuk/webpack-sourcemap-testing

There is the output:

$ webpack
Hash: 9bd50b65233535f2f320
Version: webpack 4.22.0
Time: 361ms
Built at: 2018-10-22 10:42:17
        Asset       Size  Chunks             Chunk Names
    bundle.js  984 bytes       0  [emitted]  main
bundle.js.map   4.52 KiB       0  [emitted]  main
Entrypoint main = bundle.js bundle.js.map
[0] ./src/index.js 46 bytes {0} [built]

WARNING in configuration
The 'mode' option has not been set, webpack will fallback to 'production' for this value. Set 'mode' option to 'development' or 'production' to enable defaults for each environment.
You can also set it to 'none' to disable any default behavior. Learn more: https://webpack.js.org/concepts/mode/

$ node test-sourcemaps.js
/webpack-sourcemap-testing/node_modules/sourcemap-validator/index.js:146
          throw new Error(errMsg);
          ^
Error: Warning: mismatched names
Expected: installedModules || 'installedModules' || 'installedModules' || "installedModules" || "installedModules"
Got:  	var installedM ||  	var installedMod ||  	var installedMod ||  	var installedMod ||  	var installedMod
Original Line:  	var installedModules = {};
Mapping: 1:17→2:0 "installedModules" in webpack:///webpack/bootstrap
    at /webpack-sourcemap-testing/node_modules/sourcemap-validator/index.js:146:17
    at Array.forEach (<anonymous>)
    at SourceMapConsumer_eachMapping [as eachMapping] (/webpack-sourcemap-testing/node_modules/sourcemap-validator/node_modules/source-map/lib/source-map/source-map-consumer.js:570:10)
    at validate (/webpack-sourcemap-testing/node_modules/sourcemap-validator/index.js:83:12)
    at Object.<anonymous> (/webpack-sourcemap-testing/test-sourcemaps.js:6:1)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)

cc @sokra

edmorley · 2018-10-29T16:56:34Z

did this PR fix the original issue? webpack@4.20.0 uses webpack-sources@1.3.0, but the issue is still reproducible

Would you mind opening a new issue so this doesn't get lost in the closed PR comments?

dan-codes-16 · 2018-10-29T18:06:00Z

@edmorley sure, I was going to do that.
New issue - webpack/webpack#8302
Thank you.

Connor Clark added 2 commits September 17, 2018 09:38

fix typo

1ce0fd3

fix mismatched mappings generated by OriginalSource when content cont…

7b104e5

…ains leading whitespace

alexander-akait reviewed Sep 17, 2018

View reviewed changes

alexander-akait requested review from ooflorent and sokra September 17, 2018 16:52

connorjclark commented Sep 17, 2018

View reviewed changes

remove default value syntax

72a3594

michael-ciniawsky approved these changes Sep 19, 2018

View reviewed changes

sokra added 3 commits September 20, 2018 10:19

rename oldValue to name

62e6396

add name to insert

refactor storage of replacements for readability and performance

6035781

prefer existing name over generated one

8de1165

This handles the case when identifiers are renamed twice

sokra approved these changes Sep 20, 2018

View reviewed changes

sokra merged commit 9d14ed4 into webpack:master Sep 20, 2018

dan-codes-16 mentioned this pull request Oct 19, 2018

Webpack isn't producing valid sourcemaps? webpack/webpack#7616

Closed

connorjclark mentioned this pull request Jul 26, 2019

split code into more tokens to fix sourcemap-validator warnings #51

Closed

thmsndk mentioned this pull request Dec 11, 2021

Server crash 1-2 ticks after start laverdet/xxscreeps#36

Closed

Uh oh!

Conversation

connorjclark commented Sep 17, 2018

Uh oh!

alexander-akait Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

connorjclark Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

alexander-akait Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

connorjclark Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Sep 17, 2018

Uh oh!

connorjclark commented Sep 18, 2018

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

codecov bot commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michael-ciniawsky Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

sokra commented Sep 20, 2018

Uh oh!

dan-codes-16 commented Oct 22, 2018

Uh oh!

connorjclark commented Oct 22, 2018

Uh oh!

dan-codes-16 commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edmorley commented Oct 29, 2018

Uh oh!

dan-codes-16 commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

connorjclark Sep 17, 2018 •

edited

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading

michael-ciniawsky Sep 19, 2018 •

edited

Loading

dan-codes-16 commented Oct 22, 2018 •

edited

Loading

dan-codes-16 commented Oct 29, 2018 •

edited

Loading