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

source mappings of dropped code are emitted and conflict with retained code #3001

Closed
kzc opened this issue Jul 16, 2019 · 16 comments · Fixed by #3318
Closed

source mappings of dropped code are emitted and conflict with retained code #3001

kzc opened this issue Jul 16, 2019 · 16 comments · Fixed by #3318

Comments

@kzc
Copy link
Contributor

kzc commented Jul 16, 2019

  • Rollup Version: rollup@1.17.0
  • Operating System (or Browser): any
  • Node Version: any

How Do We Reproduce?

$ yarn add rollup@1.17.0 source-map@0.5.7 --silent
$ cat -n in.js
     1	console.log(1);
     2	"unused";         // source mapping emitted
     3	console.log(2);
     4	unused => unused; // source mapping emitted
     5	console.log(3);
$ node_modules/.bin/rollup in.js -f es -m -o out.js --silent && cat -n out.js
     1	console.log(1);
     2	console.log(2);
     3	console.log(3);
     4	//# sourceMappingURL=out.js.map
$ cat source-map-dump.js
var sourcemap = JSON.parse(require("fs").readFileSync("out.js.map", "utf8"));
var consumer = new require("source-map").SourceMapConsumer(sourcemap);
consumer.eachMapping(m => console.log(JSON.stringify(m)));
$ node source-map-dump.js
{"source":"in.js","generatedLine":1,"generatedColumn":0,"originalLine":1,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":7,"originalLine":1,"originalColumn":7,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":8,"originalLine":1,"originalColumn":8,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":11,"originalLine":1,"originalColumn":11,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":12,"originalLine":1,"originalColumn":12,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":13,"originalLine":1,"originalColumn":13,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":14,"originalLine":1,"originalColumn":14,"name":null}
{"source":"in.js","generatedLine":1,"generatedColumn":15,"originalLine":1,"originalColumn":15,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":0,"originalLine":2,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":0,"originalLine":3,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":7,"originalLine":3,"originalColumn":7,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":8,"originalLine":3,"originalColumn":8,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":11,"originalLine":3,"originalColumn":11,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":12,"originalLine":3,"originalColumn":12,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":13,"originalLine":3,"originalColumn":13,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":14,"originalLine":3,"originalColumn":14,"name":null}
{"source":"in.js","generatedLine":2,"generatedColumn":15,"originalLine":3,"originalColumn":15,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":0,"originalLine":4,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":0,"originalLine":5,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":7,"originalLine":5,"originalColumn":7,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":8,"originalLine":5,"originalColumn":8,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":11,"originalLine":5,"originalColumn":11,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":12,"originalLine":5,"originalColumn":12,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":13,"originalLine":5,"originalColumn":13,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":14,"originalLine":5,"originalColumn":14,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":15,"originalLine":5,"originalColumn":15,"name":null}

Expected Behavior

Source mappings of dropped code should not be emitted.

Actual Behavior

Source mappings of dropped code are emitted:

{"source":"in.js","generatedLine":2,"generatedColumn":0,"originalLine":2,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":0,"originalLine":4,"originalColumn":0,"name":null}

which collide with these correctly generated mappings of retained code:

{"source":"in.js","generatedLine":2,"generatedColumn":0,"originalLine":3,"originalColumn":0,"name":null}
{"source":"in.js","generatedLine":3,"generatedColumn":0,"originalLine":5,"originalColumn":0,"name":null}

Encountered this bug when looking at #2996.

@lukastaegert
Copy link
Member

Interesting. I would assume this is actually not an issue within Rollup itself but in magic-string, which is generating the sourcemaps based on knowledge of all code modifications.

@lukastaegert
Copy link
Member

//cc @mourner if you have a little more insight into magic-strings internals, many recent commits were done by you.

@kzc
Copy link
Contributor Author

kzc commented Jul 17, 2019

The problem first appears in rollup@0.22.1 and magic-string@0.10.x as seen with this simplified input:

$ cat in.js
foo();
"unused";
bar();
"unused";
baz();
rm -rf package.json node_modules yarn.lock out.* && yarn add rollup@0.22.1 source-map@0.5.7 --silent && node_modules/.bin/rollup in.js -f es6 --silent --sourcemap -o out.js && cat out.js && echo && node source-map-dump.js && grep magic-string node_modules/rollup/package.json

The problem does not occur with rollup@0.22.0 and magic-string@0.8.0. It's tricky to nail down the exact magic-string version used because rollup bundles some of its dependencies. Here are the magic-string changelog entries in that range:

https://github.com/Rich-Harris/magic-string/blame/512a75e6db287eae2b77a1d5efee41c68831bd5d/CHANGELOG.md#L3-L24

In any case, the net effect is still a bug in core rollup. Other projects use labels like [bug] [blocked] [dependency] in such cases.

@kzc
Copy link
Contributor Author

kzc commented Jul 17, 2019

@Rich-Harris - any thoughts?

@kzc kzc changed the title source mappings of dropped code is emitted source mappings of dropped code are emitted Jul 17, 2019
@kzc
Copy link
Contributor Author

kzc commented Jul 20, 2019

Tentative magic-string fix for this rollup bug: Rich-Harris/magic-string#159

@kzc
Copy link
Contributor Author

kzc commented Jul 31, 2019

To see how this issue negatively impacts debugging after tree shaking, here's the rollup generated source maps in a visualizer:

rollup v1.17.0 without magic-string fix

rollup v1.17.0 with magic-string fix

Click on the [minify generated] button in the source map visualization of each link to see the impact for downstream users of rollup source maps. Hover over or click the original source pane's unused dropped code after minification is selected.

@kzc kzc changed the title source mappings of dropped code are emitted source mappings of dropped code are emitted and conflict with retained code Aug 13, 2019
mourner pushed a commit to Rich-Harris/magic-string that referenced this issue Oct 4, 2019
Fixes #149

Also fixes two incorrect test results and the rollup issue:
rollup/rollup#3001
@kzc
Copy link
Contributor Author

kzc commented Jan 2, 2020

magic-string@0.25.5 was just released with a number of fixes including the one pertinent to this issue. Because magic-string is inlined into the Rollup bundle, users will have to wait until Rollup dependencies are updated and a new release is made.

Be aware that upgrading to the new version of magic-string will likely alter every sourcemap checked into the rollup tree.

Closing issue.

@lukastaegert
Copy link
Member

Thanks for the tip, I created #3318 to update the dependency

@kzc kzc reopened this Jan 11, 2020
@kzc
Copy link
Contributor Author

kzc commented Jan 11, 2020

The source map fix in Rich-Harris/magic-string#159 wasn't general enough. It only applied to the first source file. Here's a test case demonstrating incorrect mappings with latest rollup:

$ cat in2.js 
import "./in3.js";
console.log(1);
"unused";         // source mapping emitted
console.log(2);
unused => unused; // source mapping emitted
console.log(3);
$ cat in3.js 
console.log(4);
"unused3";
console.log(5);
unused3 => unused3;
console.log(6);
$ dist/bin/rollup -v
rollup v1.29.0
$ dist/bin/rollup in2.js -m inline -o out2.js

in2.js → out2.js...
created out2.js in 61ms
$ cat out2.js 
console.log(4);
console.log(5);
console.log(6);

console.log(1);
console.log(2);
console.log(3);
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoib3V0Mi5qcyIsInNvdXJjZXMiOlsiaW4zLmpzIiwiaW4yLmpzIl0sInNvdXJjZXNDb250ZW50IjpbImNvbnNvbGUubG9nKDQpO1xuXCJ1bnVzZWQzXCI7XG5jb25zb2xlLmxvZyg1KTtcbnVudXNlZDMgPT4gdW51c2VkMztcbmNvbnNvbGUubG9nKDYpO1xuIiwiaW1wb3J0IFwiLi9pbjMuanNcIjtcbmNvbnNvbGUubG9nKDEpO1xuXCJ1bnVzZWRcIjsgICAgICAgICAvLyBzb3VyY2UgbWFwcGluZyBlbWl0dGVkXG5jb25zb2xlLmxvZygyKTtcbnVudXNlZCA9PiB1bnVzZWQ7IC8vIHNvdXJjZSBtYXBwaW5nIGVtaXR0ZWRcbmNvbnNvbGUubG9nKDMpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUFBLE9BQU8sQ0FBQyxHQUFHLENBQUMsQ0FBQyxDQUFDLENBQUM7QUFFZixPQUFPLENBQUMsR0FBRyxDQUFDLENBQUMsQ0FBQyxDQUFDO0FBRWYsT0FBTyxDQUFDLEdBQUcsQ0FBQyxDQUFDLENBQUM7O0FDSGQsT0FBTyxDQUFDLEdBQUcsQ0FBQyxDQUFDLENBQUMsQ0FBQztBQUNmLEFBQ0EsT0FBTyxDQUFDLEdBQUcsQ0FBQyxDQUFDLENBQUMsQ0FBQztBQUNmLEFBQ0EsT0FBTyxDQUFDLEdBQUcsQ0FBQyxDQUFDLENBQUMsQ0FBQyJ9

Here's the bug viewed with sourcemap visualizer

Still more bugs are evident when the [minify generated] button is clicked in source map visualizer.

magic-string fix in the works.

@kzc
Copy link
Contributor Author

kzc commented Jan 11, 2020

Confirmed that magic-string PR Rich-Harris/magic-string#173 fixes this multi-source bundle mapping bug. Once merged and magic-string released expect to update most source maps in the rollup tree once again.

@kzc
Copy link
Contributor Author

kzc commented Jan 19, 2020

The magic-string PR in question was merged. Once a release is made Rollup can update its deps. Closing out issue.

@kzc kzc closed this as completed Jan 19, 2020
@lukastaegert
Copy link
Member

Thanks for your work on this ♥️ I created Rich-Harris/magic-string#174 so that we become less dependent on the magic-string release cycle.

@kzc
Copy link
Contributor Author

kzc commented Jan 21, 2020

@lukastaegert Happy to help out.

Do you plan to use non-released magic-string commits and/or PRs in a future Rollup release, or is this just for your own local testing?

Side note - I'm surprised that Rollup users didn't notice the rather obvious source map bugs. It appears that fewer users actually do source map debugging than I thought.

@lukastaegert
Copy link
Member

Do you plan to use non-released magic-string commits and/or PRs in a future Rollup release

I wanted to but it turned out that npm audit fails miserably if the lock file contains direct Github references 🙄. And since I do not want to disable CI for this, I guess I'll wait for a release. But at least local testing is now easily possible.

@kzc
Copy link
Contributor Author

kzc commented Jan 23, 2020

Might have to wait a few months for a release.

If magic-string was an external module instead of bundled into Rollup, users could use yarn resolutions to replace the module.

@lukastaegert You might consider creating a @rollup/magic-string fork for the v2.0.0 release as this library is rather integral to Rollup.

@lukastaegert
Copy link
Member

Nah, I'll just nag people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants