inline sourcemaps and cache API cause incorrect sourcemap output

[jbedard]

• Rollup Version: 2.2.0, pretty sure it also occured in 1.3
• Operating System (or Browser): many via CI

Reproduction (can create a git repo if wanted, but this is pretty simple):

Given the following inputs:

index.js

import value from "./other";
console.log(value);

other.js (compiled from a ts file that used const and had x: string)

var x = "foo";
export default x;
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoib3RoZXIuanMiLCJzb3VyY2VSb290IjoiIiwic291cmNlcyI6WyJvdGhlci50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxJQUFNLENBQUMsR0FBVyxLQUFLLENBQUM7QUFDeEIsZUFBZSxDQUFDLENBQUMifQ==

Then run rollup via the API twice, the second time with the cache of the first. This is essentially the same as https://rollupjs.org/guide/en/#cache

const rollup = require('rollup');

let cache;
let runNum = 1;
function run() {
    return rollup.rollup({cache, input: ["./index.js"]});
}
function write(bundle) {
    cache = bundle.cache;
    return bundle.write({file: `out/run-${runNum++}.js`})
}

Promise.resolve()
    .then(run).then(write)
    .then(run).then(write);

Expected Behavior

The same output each time (a single .js.map file alongside the bundle). Hopefully the cache makes it faster.

Actual Behavior

The //# sourceMappingURL=data: correctly gets merged into the outputted run-1.js.map on the first run. Then it remains within the code (run-2.js) on the second run (and run-2.js.map is a tiny amount larger then the first run, so it's probably still merged into there).

[jbedard]

Here are a few tests reproducing it: jbedard@5765190

[jbedard]

Note this also happens when using --watch, so the CLI usage of this API has the same bug.

[lukastaegert]

Here are a few tests reproducing it: jbedard/rollup@5765190

Your test does not appear to use the cache API, am I missing something here?

So one thing to note is that Rollup does NOT pick up inlined or external sourcemaps by default to merge them into the final sourcemap. If you want something like this, you may want e.g. https://github.com/maxdavidson/rollup-plugin-sourcemaps

Beyond that it sounds like your actual problem is that the inline sourcemap is not removed and thus causes issues, is that correct?

[lukastaegert]

But I can definitely confirm that the sourcemap is only removed on the first run and not on the second, so it appears the existing logic to remove sourcemaps is not triggered when using the cache. We definitely need to fix this!

[jbedard]

Your test does not appear to use the cache API, am I missing something here?

Every test in the sourcemaps examples is run twice using the cache, ironically to make sure you get the same result each time :)

[lukastaegert]

Ah, nicely spotted!

[lukastaegert]

Tentative fix at #3476

[jbedard]

Thanks!

Worth adding jbedard@5765190 as well?

[lukastaegert]

Sure, will try to merge it

[lukastaegert]

The test description is wrong, though, as the inline sourcemaps are not combined. They are just removed. See my comment above.

[lukastaegert]

And we will need to switch to asserts that work in Node 10...

[jbedard]

The test description is wrong, though, as the inline sourcemaps are not combined

I'm confused... the tests are outputting sourcemaps though (there are .js.map files in the _action output folder). Doesn't the outputOptions.sourcemap = true output sourcemaps?

[lukastaegert]

Yes it does. But the sourcemaps only take Rollup's transformations + any plugin transformations into account. Pre-existing sourcemaps are ignored by default — unless you add a plugin that changes that. The reasoning is that developers are usually only interested in sourcemaps for their own code, and plugins take care of that. Pre-existing sourcemaps usually come with third-party libraries where you are usually not that interested in detailed original code locations, especially if you do not have this code available anyway. Also, obeying pre-existing sourcemaps comes at a performance cost.

[lukastaegert]

I removed the test about external sourcemaps because it really does not test anything interesting IMO, and change the description and asserts of the other test.

[jbedard]

the sourcemaps only take Rollups transformations + any plugin transformations into account. Pre-existing sourcemaps are ignored by default — unless you add a plugin that changes that

I see, thanks!

This look right? jbedard@3e18939

[lukastaegert]

Yes. But the two tests now basically test the same thing, so I would remove one.

[jbedard]

I figured it was worth testing inline + external. I had to test both to know if it effected both 🤷‍♂

I can remove one if you prefer that though. Remove the inline one because a giant blob of base64 is ugly..?

[lukastaegert]

Well, only inline sourcemaps are a problem. External sourcemaps are not touched at all.

[jbedard]

But the sourcemap comment should be removed either way...

[lukastaegert]

Ah true, there is still a comment. Makes sense.