🏗 Fix sourcemap sources paths (#27665)

* Fix sourcemap sources paths

* Review updates

* Separate makeSourcemapsRelative into a helper function

* Allow sourcemap_url without fortesting

build-system/compile/closure-compile.js

@@ -18,6 +18,9 @@
 const argv = require('minimist')(process.argv.slice(2));
 const closureCompiler = require('google-closure-compiler');
 const log = require('fancy-log');
+const path = require('path');
+const pumpify = require('pumpify');
+const sourcemaps = require('gulp-sourcemaps');
 const {cyan, red, yellow} = require('ansi-colors');
 const {EventEmitter} = require('events');
 const {highlight} = require('cli-highlight');
@@ -100,6 +103,26 @@ function logError(message) {
   log(`${message}\n` + formatClosureCompilerError(compilerErrors));
 }
 
+/**
+ * Normalize the sourcemap file paths before pushing into Closure.
+ * Closure don't follow Gulp's normal sourcemap "root" pattern. Gulp considers
+ * all files to be relative to the CWD by default, meaning a file `src/foo.js`
+ * with a sourcemap alongside points to `src/foo.js`. Closure considers each
+ * file relative to the sourcemap. Since the sourcemap for `src/foo.js` "lives"
+ * in `src/`, it ends up resolving to `src/src/foo.js`.
+ *
+ * @param {!Stream} closureStream
+ * @return {!Stream}
+ */
+function makeSourcemapsRelative(closureStream) {
+  const relativeSourceMap = sourcemaps.mapSources((source, file) => {
+    const dir = path.dirname(file.sourceMap.file);
+    return path.relative(dir, source);
+  });
+
+  return pumpify.obj(relativeSourceMap, closureStream);
+}
+
 /**
  * @param {Array<string>} compilerOptions
  * @param {?string} nailgunPort
@@ -144,7 +167,9 @@ function gulpClosureCompile(compilerOptions, nailgunPort) {
     }
   }
 
-  return closureCompiler.gulp(initOptions)(compilerOptions, pluginOptions);
+  return makeSourcemapsRelative(
+    closureCompiler.gulp(initOptions)(compilerOptions, pluginOptions)
+  );
 }
 
 module.exports = {

build-system/compile/compile.js

@@ -37,8 +37,8 @@ const {postClosureBabel} = require('./post-closure-babel');
 const {preClosureBabel, handlePreClosureError} = require('./pre-closure-babel');
 const {singlePassCompile} = require('./single-pass');
 const {VERSION: internalRuntimeVersion} = require('./internal-version');
+const {writeSourcemaps} = require('./helpers');
 
-const isProdBuild = !!argv.type;
 const queue = [];
 let inProgress = 0;
 
@@ -116,19 +116,6 @@ function compile(
     );
   }
 
-  function getSourceMapBase() {
-    if (isProdBuild) {
-      return `https://raw.githubusercontent.com/ampproject/amphtml/${internalRuntimeVersion}/`;
-    } else if (argv.sourcemap_url) {
-      // Custom sourcemap URLs have placeholder {version} that should be
-      // replaced with the actual version. Also, ensure trailing slash exists.
-      return String(argv.sourcemap_url)
-        .replace(/\{version\}/g, internalRuntimeVersion)
-        .replace(/([^/])$/, '$1/');
-    }
-    return 'http://localhost:8000/';
-  }
-
   const hideWarningsFor = [
     'third_party/amp-toolbox-cache-url/',
     'third_party/caja/',
@@ -194,7 +181,6 @@ function compile(
     if (options.wrapper) {
       wrapper = options.wrapper.replace('<%= contents %>', '%output%');
     }
-    const sourceMapBase = getSourceMapBase();
     const srcs = [...CLOSURE_SRC_GLOBS];
     // Add needed path for extensions.
     // Instead of globbing all extensions, this will only add the actual
@@ -307,7 +293,6 @@ function compile(
       dependency_mode: 'PRUNE',
       output_wrapper: wrapper,
       source_map_include_content: !!argv.full_sourcemaps,
-      source_map_location_mapping: '|' + sourceMapBase,
       warning_level: options.verboseLogging ? 'VERBOSE' : 'DEFAULT',
       // These arrays are filled in below.
       jscomp_error: [],
@@ -395,23 +380,23 @@ function compile(
         .on('error', (err) =>
           handleCompilerError(err, outputFilename, options, resolve)
         )
-        .pipe(rename(outputFilename))
+        .pipe(rename(`${outputDir}/${outputFilename}`))
         .pipe(
           gulpIf(
             !argv.pseudo_names && !options.skipUnknownDepsCheck,
             checkForUnknownDeps()
           )
         )
         .on('error', reject)
-        .pipe(sourcemaps.write('.'))
         .pipe(
           gulpIf(
             shouldAppendSourcemappingURLText,
             gap.appendText(`\n//# sourceMappingURL=${outputFilename}.map`)
           )
         )
-        .pipe(postClosureBabel(outputDir))
-        .pipe(gulp.dest(outputDir))
+        .pipe(postClosureBabel())
+        .pipe(writeSourcemaps())
+        .pipe(gulp.dest('.'))
         .on('end', resolve);
     }
   });

build-system/compile/helpers.js

@@ -0,0 +1,46 @@
+/**
+ * Copyright 2020 The AMP HTML Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+'use strict';
+
+const minimist = require('minimist');
+const sourcemaps = require('gulp-sourcemaps');
+const {VERSION: internalRuntimeVersion} = require('./internal-version');
+
+const argv = minimist(process.argv.slice(2));
+
+function getSourceMapBase() {
+  if (argv.sourcemap_url) {
+    // Custom sourcemap URLs have placeholder {version} that should be
+    // replaced with the actual version. Also, ensure trailing slash exists.
+    return String(argv.sourcemap_url)
+      .replace(/\{version\}/g, internalRuntimeVersion)
+      .replace(/([^/])$/, '$1/');
+  }
+  if (argv.fortesting) {
+    return 'http://localhost:8000/';
+  }
+  return `https://raw.githubusercontent.com/ampproject/amphtml/${internalRuntimeVersion}/`;
+}
+
+function writeSourcemaps() {
+  return sourcemaps.write('.', {
+    sourceRoot: getSourceMapBase(),
+  });
+}
+
+module.exports = {
+  writeSourcemaps,
+};

build-system/compile/post-closure-babel.js

@@ -16,41 +16,12 @@
 'use strict';
 const argv = require('minimist')(process.argv.slice(2));
 const babel = require('@babel/core');
-const fs = require('fs-extra');
 const path = require('path');
 const remapping = require('@ampproject/remapping');
 const terser = require('terser');
 const through = require('through2');
 const {debug, CompilationLifecycles} = require('./debug-compilation-lifecycle');
 
-/**
- * Given a filepath, return the sourcemap.
- *
- * @param {string} file
- * @return {string|null}
- */
-function loadSourceMap(file) {
-  if (file.startsWith('dist')) {
-    return fs.readFile(`${file}.map`);
-  }
-  return null;
-}
-
-/**
- * @param {string} map
- * @return {function(string)}
- */
-function returnMapFirst(map) {
-  let first = true;
-  return function (file) {
-    if (first) {
-      first = false;
-      return map;
-    }
-    return loadSourceMap(file);
-  };
-}
-
 /**
  * Minify passed string.
  *
@@ -83,49 +54,40 @@ function terserMinify(code) {
  * Apply Babel Transforms on output from Closure Compuler, then cleanup added
  * space with Terser. Used only in esm mode.
  *
- * @param {string} directory directory this file lives in
  * @return {!Promise}
  */
-exports.postClosureBabel = function (directory) {
+exports.postClosureBabel = function () {
   return through.obj(function (file, enc, next) {
     if (!argv.esm || path.extname(file.path) === '.map') {
       return next(null, file);
     }
 
+    const map = file.sourceMap;
+
     debug(
       CompilationLifecycles['closured-pre-babel'],
       file.path,
       file.contents
     );
-    const map = loadSourceMap(file.path);
     const {code, map: babelMap} = babel.transformSync(file.contents, {
       caller: {name: 'post-closure'},
     });
+
     debug(
       CompilationLifecycles['closured-pre-terser'],
       file.path,
       file.contents
     );
-    let remapped = remapping(
-      babelMap,
-      returnMapFirst(map),
-      !argv.full_sourcemaps
-    );
-
     const {compressed, terserMap} = terserMinify(code);
-    file.contents = Buffer.from(compressed, 'utf-8');
+
     debug(CompilationLifecycles['complete'], file.path, compressed);
 
-    // TODO: Remapping should support a chain, instead of multiple invocations.
-    remapped = remapping(
-      terserMap,
-      returnMapFirst(remapped),
+    file.contents = Buffer.from(compressed, 'utf-8');
+    file.sourceMap = remapping(
+      [terserMap, babelMap, map],
+      () => null,
       !argv.full_sourcemaps
     );
-    fs.writeFileSync(
-      path.resolve(directory, `${path.basename(file.path)}.map`),
-      remapped.toString()
-    );
 
     return next(null, file);
   });