PRO-8803: sourcemaps fixed all the ways (#5217)

* PRO-8803: implement productionSourceMaps properly, and add productionSourceMapDir

* working nicely in all scenarios

* lint clean

* line breaks are good

* fixed dev sourcemaps too, and we can use devtool: 'source-map' everywhere because we've solved for the underlying problem we had all along

* unused variables

* I busted the no sourcemap case

Author: boutell

.changeset/quick-phones-care.md

@@ -0,0 +1,6 @@
+---
+"apostrophe": patch
+---
+
+- The `productionSourceMaps` option is now fully supported in both Vite and Webpack. Previously this feature did not work fully in Vite, and was not supported with Webpack. Enabling this feature completes the task of making sourcemaps fully available in the browser in production.
+- If you want production sourcemaps to be created but not actually uploaded for the public, you can combine `productionSourceMaps: true` with the new `productionSourceMapsDir` option to specify an alternate location where the `@apostrophecms/asset:build` task should place them. By using this option, you take responsibility for delivering the sourcemaps to their final home. Creating and/or erasing the folder between builds is also up to you. Most people will not need this option.

packages/apostrophe/modules/@apostrophecms/asset/index.js

@@ -84,10 +84,14 @@ module.exports = {
       transform: null
     },
     // If true, the source maps will be generated in production.
-    // This option is useful for debugging in production and is only
-    // available when an external build module is registered (it doesn't
-    // with the internal webpack build).
+    // This option is useful for debugging in production
     productionSourceMaps: false,
+    // By default, productionSourceMaps: true will push the source maps live right
+    // alongside the bundle files, allowing sources to be inspected in production.
+    // If you would like to send your production sourcemaps somewhere else,
+    // specify productionSourceMapsDir. After that, doing something with the
+    // sourcemaps is entirely up to you
+    productionSourceMapsDir: null,
     // The configuration to control the development server and HMR when
     // supported. The value can be: - boolean `false`: disable the dev server and
     // HMR. - boolean `true`: same as `public` (default). - string `public`:
@@ -582,13 +586,12 @@ module.exports = {
         const deployableArtefacts = await self.copyBuildArtefacts(
           self.currentBuildManifest
         );
-        // Copy the source maps to the bundle root.
-        const sourceMaps = await self.copyBuildSourceMaps(self.currentBuildManifest);
         // Save the build manifest in the bundle root.
         await self.saveBuildManifest(self.currentBuildManifest);
 
         // Deploy everything to the release location.
         // All paths are relative to the bundle root.
+
         const deployFiles = [
           ...new Set(
             [
@@ -598,8 +601,8 @@ module.exports = {
             ]
           )
         ];
-        if (self.options.productionSourceMaps) {
-          deployFiles.push(...sourceMaps || []);
+        if (self.options.productionSourceMaps && !self.options.productionSourceMapsDir) {
+          deployFiles.push(...bundles.map(bundle => `${bundle}.map`));
         }
 
         await self.deploy(deployFiles);

packages/apostrophe/modules/@apostrophecms/asset/lib/build/internals.js

@@ -3,6 +3,7 @@ const path = require('node:path');
 const util = require('node:util');
 const { glob } = require('../../lib/path');
 const { getBuildExtensions, fillExtraBundles } = require('./utils');
+const Concat = require('concat-with-sourcemaps');
 
 // Internal build interface.
 module.exports = (self) => {
@@ -207,6 +208,8 @@ module.exports = (self) => {
     // external build module build method (see `self.build()` and
     // `configureBuildModule()`).
     async computeBuildScenes(metadata, { write = true } = {}) {
+
+      const needSourceMap = self.options.productionSourceMaps;
       const bundlePath = self.getBundleRootDir();
       const buildRoot = self.getBuildRootDir();
 
@@ -266,26 +269,65 @@ module.exports = (self) => {
         if (!write) {
           return Object.keys(bundles);
         }
+
         for (const [ target, files ] of Object.entries(bundles)) {
+
+          let content = null;
+          let sourceMap = null;
+
           if (!files.length) {
             delete bundles[target];
             continue;
           }
-          const content = files.map(f =>
-            fs.existsSync(f)
-              ? `\n\n/** ${path.basename(f)} **/\n\n` + fs.readFileSync(f, 'utf-8')
-              : ''
-          )
-            .join('\n')
-            .trim();
+
+          const filePath = path.join(bundlePath, target);
+
+          const fileName = filePath.split('/').at(-1);
+          if (needSourceMap) {
+
+            // Concatenate in a way that preserves sitemaps
+            const concat = new Concat(true, bundlePath, '\n');
+            for (const file of files) {
+              const map = `${file}.map`;
+              // concat-with-sourcemaps does not strip old sourcemap comments for us
+              const source = stripSourceMapComment(fs.readFileSync(file, 'utf8'));
+              if (!fs.existsSync(map)) {
+                concat.add(
+                  file,
+                  source
+                );
+              } else {
+                concat.add(
+                  file,
+                  source,
+                  // Per docs for concat-source-maps: this one should be read as a string
+                  fs.readFileSync(map, 'utf8')
+                );
+              }
+            }
+            content = concat.content.toString('utf8') + `\n//# sourceMappingURL=${fileName}.map\n`;
+            sourceMap = concat.sourceMap;
+          } else {
+            content = files.map(f => fs.readFileSync(f, 'utf-8'))
+              .join('\n')
+              .trim();
+          }
           if (!content.trim().length) {
             delete bundles[target];
             continue;
           }
-          fs.outputFileSync(
-            path.join(bundlePath, target),
+          fs.writeFileSync(
+            filePath,
             content
           );
+          if (sourceMap != null) {
+            const dir = self.options.productionSourceMapsDir;
+            const sourceMapPath = dir ? `${dir}/${fileName}.map` : `${filePath}.map`;
+            fs.writeFileSync(
+              sourceMapPath,
+              sourceMap
+            );
+          }
         }
 
         return Object.keys(bundles);
@@ -550,3 +592,8 @@ module.exports = (self) => {
     }
   };
 };
+
+// Helper to remove existing sourceMappingURL comments
+function stripSourceMapComment(code) {
+  return code.replace(/\/\/[@#]\s*sourceMappingURL=.*$/gm, '');
+}

packages/apostrophe/modules/@apostrophecms/asset/lib/build/task.js

@@ -11,6 +11,7 @@ const {
   writeBundlesImportFiles,
   findNodeModulesSymlinks
 } = require('../webpack/utils');
+const Concat = require('concat-with-sourcemaps');
 
 // The internal Webpack build task.
 module.exports = (self) => ({
@@ -544,22 +545,52 @@ module.exports = (self) => ({
                 scenes.includes(scene);
       };
 
-      const filesContent = Object.entries(self.builds)
+      const files = Object.entries(self.builds)
         .filter(([ _, options ]) => filterBuilds(options))
-        .map(([ name ]) => {
-          const file = `${bundleDir}/${name}-build.${fileExt}`;
-          const readFile = (n, f) => `/* BUILD: ${n} */\n${fs.readFileSync(f, 'utf8')}`;
-
-          if (checkForFile) {
-            return fs.existsSync(file)
-              ? readFile(name, file)
-              : '';
-          }
+        .map(([ name ]) => `${bundleDir}/${name}-build.${fileExt}`)
+        .filter(file => {
+          return (!checkForFile) || fs.existsSync(file);
+        });
+
+      const bundlePath = `${bundleDir}/${filePath}`;
+      const sourceMapDir = self.options.productionSourceMapsDir || bundleDir;
+      const sourceMapPath = `${sourceMapDir}/${filePath}.map`;
+
+      const needSourcemap = (process.env.NODE_ENV !== 'production') || self.options.productionSourceMaps;
+
+      if (!needSourcemap) {
+        return fs.writeFileSync(bundlePath, files.map(file => fs.readFileSync(file, 'utf8')).join('\n'));
+      }
+
+      // Concatenate in a way that preserves sitemaps
+      const concat = new Concat(true, bundlePath, '\n');
+
+      for (const file of files) {
+        const map = `${file}.map`;
+        // concat-with-sourcemaps does not strip old sourcemap comments for us
+        const source = stripSourceMapComment(fs.readFileSync(file, 'utf8'));
+        if (!fs.existsSync(map)) {
+          concat.add(
+            file,
+            source
+          );
+        } else {
+          concat.add(
+            file,
+            source,
+            // Per docs for concat-source-maps: this one should be read as a string
+            fs.readFileSync(map, 'utf8')
+          );
+        }
+      }
 
-          return readFile(name, file);
-        }).join('\n');
+      // Normally a production build does not include a sourcemap in the
+      // final deliverable
 
-      fs.writeFileSync(`${bundleDir}/${filePath}`, filesContent);
+      // concat-with-sourcemaps does not do this either
+      const content = concat.content.toString('utf8') + `\n//# sourceMappingURL=${filePath}.map\n`;
+      fs.writeFileSync(bundlePath, content);
+      fs.writeFileSync(sourceMapPath, concat.sourceMap);
     }
 
     // If NODE_ENV is production, this function will copy the given
@@ -596,7 +627,12 @@ module.exports = (self) => ({
       for (const file of files) {
         const src = `${bundleDir}/${file}`;
         await copyIn(src, `${releaseDir}/${file}`);
+        const srcMap = `${src}.map`;
+        if (fs.existsSync(srcMap)) {
+          await copyIn(srcMap, `${releaseDir}/${file}.map`);
+        }
         await fs.remove(src);
+        await fs.remove(srcMap);
       }
     }
 
@@ -864,3 +900,8 @@ module.exports = (self) => ({
     }
   }
 });
+
+// Helper to remove existing sourceMappingURL comments
+function stripSourceMapComment(code) {
+  return code.replace(/\/\/[@#]\s*sourceMappingURL=.*$/gm, '');
+}

packages/apostrophe/modules/@apostrophecms/asset/lib/webpack/apos/webpack.config.js

@@ -24,15 +24,6 @@ module.exports = ({
 
   const mode = process.env.NODE_ENV || 'development';
   const pnpmModulePath = apos.isPnpm ? [ path.join(apos.selfDir, '../') ] : [];
-  const dev = (mode === 'development');
-  // The default behavior of the security-headers module
-  // blocks eval, and only eval-source-map currently associates
-  // errors with the right file. With regular source-map the
-  // results are worse than no source map. A developer who wishes
-  // to experiment can set the devSourceMap option of the asset module
-  const csp = apos.modules['@apostrophecms/security-headers'];
-  const fallbackSourceMap = csp ? false : 'eval-source-map';
-  const devtool = dev ? (apos.asset.options.devSourceMap || fallbackSourceMap) : false;
   const config = {
     performance: {
       hints: false
@@ -44,7 +35,7 @@ module.exports = ({
     optimization: {
       minimize: process.env.NODE_ENV === 'production'
     },
-    devtool,
+    devtool: 'source-map',
     output: {
       path: outputPath,
       filename: outputFilename

packages/apostrophe/package.json

@@ -70,6 +70,7 @@
     "cheerio": "^1.0.0-rc.10",
     "chokidar": "^3.5.2",
     "common-tags": "^1.8.0",
+    "concat-with-sourcemaps": "^1.1.0",
     "connect-mongo": "^5.1.0",
     "cookie-parser": "^1.4.5",
     "cors": "^2.8.5",