Store locations for removed comments in cache (#3476)

* add tests for merging sourcemaps

* Store locations for removed comments in cache

* Fix tests

* Fix tests

Co-authored-by: Jason Bedard <jason+github@jbedard.ca>

src/Module.ts

@@ -236,6 +236,7 @@ export default class Module {
 	usesTopLevelAwait = false;
 
 	private allExportNames: Set<string> | null = null;
+	private alwaysRemovedCode!: [number, number][];
 	private ast!: Program;
 	private astContext!: AstContext;
 	private context: string;
@@ -619,6 +620,7 @@ export default class Module {
 	}
 
 	setSource({
+		alwaysRemovedCode,
 		ast,
 		code,
 		customTransformCache,
@@ -631,6 +633,7 @@ export default class Module {
 		transformDependencies,
 		transformFiles
 	}: TransformModuleJSON & {
+		alwaysRemovedCode?: [number, number][];
 		transformFiles?: EmittedFile[] | undefined;
 	}) {
 		this.code = code;
@@ -651,8 +654,18 @@ export default class Module {
 
 		timeStart('generate ast', 3);
 
-		this.esTreeAst = ast || tryParse(this, this.graph.acornParser, this.graph.acornOptions);
-		markPureCallExpressions(this.comments, this.esTreeAst);
+		this.alwaysRemovedCode = alwaysRemovedCode || [];
+		if (ast) {
+			this.esTreeAst = ast;
+		} else {
+			this.esTreeAst = tryParse(this, this.graph.acornParser, this.graph.acornOptions);
+			for (const comment of this.comments) {
+				if (!comment.block && SOURCEMAPPING_URL_RE.test(comment.text)) {
+					this.alwaysRemovedCode.push([comment.start, comment.end]);
+				}
+			}
+			markPureCallExpressions(this.comments, this.esTreeAst);
+		}
 
 		timeEnd('generate ast', 3);
 
@@ -666,7 +679,9 @@ export default class Module {
 			filename: (this.excludeFromSourcemap ? null : fileName)!, // don't include plugin helpers in sourcemap
 			indentExclusionRanges: []
 		});
-		this.removeExistingSourceMap();
+		for (const [start, end] of this.alwaysRemovedCode) {
+			this.magicString.remove(start, end);
+		}
 
 		timeStart('analyse ast', 3);
 
@@ -720,6 +735,7 @@ export default class Module {
 
 	toJSON(): ModuleJSON {
 		return {
+			alwaysRemovedCode: this.alwaysRemovedCode,
 			ast: this.esTreeAst,
 			code: this.code,
 			customTransformCache: this.customTransformCache,
@@ -912,14 +928,6 @@ export default class Module {
 		}
 	}
 
-	private removeExistingSourceMap() {
-		for (const comment of this.comments) {
-			if (!comment.block && SOURCEMAPPING_URL_RE.test(comment.text)) {
-				this.magicString.remove(comment.start, comment.end);
-			}
-		}
-	}
-
 	private shimMissingExport(name: string): void {
 		if (!this.exports[name]) {
 			this.graph.warn({

src/rollup/types.d.ts

@@ -94,7 +94,7 @@ export interface SourceDescription {
 }
 
 export interface TransformModuleJSON {
-	ast: AcornNode;
+	ast?: AcornNode;
 	code: string;
 	// note if plugins use new this.cache to opt-out auto transform cache
 	customTransformCache: boolean;
@@ -108,6 +108,8 @@ export interface TransformModuleJSON {
 }
 
 export interface ModuleJSON extends TransformModuleJSON {
+	alwaysRemovedCode: [number, number][];
+	ast: AcornNode;
 	dependencies: string[];
 	id: string;
 	transformFiles: EmittedFile[] | undefined;

src/utils/transform.ts

@@ -160,7 +160,7 @@ export default function transform(
 			}
 
 			return {
-				ast: ast!,
+				ast,
 				code,
 				customTransformCache,
 				moduleSideEffects,

test/misc/misc.js

@@ -108,12 +108,10 @@ describe('misc', () => {
 			.then(bundle => bundle.generate({ format: 'es' }))
 			.then(({ output }) => {
 				assert.equal(warnings.length, 0);
-				assert.deepEqual(output.map(({ fileName }) => fileName), [
-					'main1.js',
-					'main2.js',
-					'dep-f8bec8a7.js',
-					'dyndep-b0a9ee12.js'
-				]);
+				assert.deepEqual(
+					output.map(({ fileName }) => fileName),
+					['main1.js', 'main2.js', 'dep-f8bec8a7.js', 'dyndep-b0a9ee12.js']
+				);
 			});
 	});
 
@@ -175,4 +173,38 @@ describe('misc', () => {
 				assert.strictEqual(output.output[0].code, 'var input = 42;\n\nexport default input;\n')
 			);
 	});
+
+	it('consistently handles comments when using the cache', async () => {
+		const FILES = {
+			main: `import value from "other";
+console.log(value);
+/*#__PURE__*/console.log('removed');`,
+			other: `var x = "foo";
+export default x;
+//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoib3RoZXIuanMiLCJzb3VyY2VSb290IjoiIiwic291cmNlcyI6WyJvdGhlci50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxJQUFNLENBQUMsR0FBVyxLQUFLLENBQUM7QUFDeEIsZUFBZSxDQUFDLENBQUMifQ==`
+		};
+		const EXPECTED_OUTPUT = `var x = "foo";
+
+console.log(x);
+`;
+		const firstBundle = await rollup.rollup({
+			input: 'main',
+			plugins: [loader(FILES)]
+		});
+		assert.strictEqual(
+			(await firstBundle.generate({ format: 'es' })).output[0].code,
+			EXPECTED_OUTPUT,
+			'first'
+		);
+		const secondBundle = await rollup.rollup({
+			cache: firstBundle.cache,
+			input: 'main',
+			plugins: [loader(FILES)]
+		});
+		assert.strictEqual(
+			(await secondBundle.generate({ format: 'es' })).output[0].code,
+			EXPECTED_OUTPUT,
+			'second'
+		);
+	});
 });

test/sourcemaps/samples/existing-external-sourcemaps-combined/_config.js

@@ -0,0 +1,8 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'removes sourcemap comments',
+	async test(code) {
+		assert.strictEqual(code.indexOf('sourceMappingURL'), -1);
+	}
+};

test/sourcemaps/samples/existing-inline-sourcemaps-combined/_config.js

@@ -0,0 +1,8 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'removes existing inline sourcemaps',
+	async test(code) {
+		assert.strictEqual(code.indexOf('sourceMappingURL'), -1);
+	}
+};