prepend-config should use magic-string instead of normal cat

[erwinmombay]

magic-string has the ability to update the source map files so it is better than a normal cat

[erwinmombay]

https://github.com/Rich-Harris/magic-string

[Marcial1234]

Hey @erwinmombay, do you have any more documentation/requirements on this?

Is this only on build-system/single-pass.js under wrapMainBinaries, or also in gulpfile.js under the f(x)s checkTypes and appendToCompiledFile?

[Marcial1234]

Will ping tomorrow for a quick walk thru.

Need to know:

• Which gulp trigger calls the code inside single-pass. Code exported as part of tasks/index.js, but I lose track after.
• Any way to test more than one JS at the time? So far the ones I build with gulp dist don't have source map issues.

[erwinmombay]

Which gulp trigger calls the code inside single-pass. Code exported as part of tasks/index.js, but I lose track after.

• gulp dist --single_pass is the command to run single pass

Any way to test more than one JS at the time? So far the ones I build with gulp dist don't have source map issues.

• hmm that's interesting, it should always generate source maps (.js.map files)

feel free to ping me anytime or schedule a call

[Marcial1234]

Source Map issue summary

• The way that we're bundling JavaScript files tends to break their source map links.
• Other source map issues could be a Node problem (source maps are not working when debugging with chrome nodejs/node#8369) when debugging served files locally.

Walk-thru example by @danielrozenberg

• On the official AMP page browse to a minified AMP javascript file
• On the top a "source map detected" header should appear
• Click Ctrl+P to browse thru the page resources
• Type in the name of the minified AMP js file, and see if there's a version-less version it, but coming from github (AMP site) or localhost (locally)
  • If so, click it and you should be to see the original code, put breakpoints, and debug that external file
  • If it doesn't show up, that file's Source Map isn't working
• Repeat this step, but for locally-built files using gulp dist --single_pass

Work Summary

• Was stuck a bit thinking nothing worked locally, until I figured I had to run a server thru gulp
• Painstakingly checked ALL .js files that output a .js.map to see if they showed their source/server counter part. All /v0 files had their source show as standalone imports. The others were:
  • dist/examiner.js
  • dist/video-iframe-integration-v0.js
• Observed all .max.js files aren't minified
• The files with issues are the bundled files modified in build-systems/single-pass.js@wrapMainBinaries, which are:
  • dist/amp4ads-v0.js
  • dist/shadow-v0.js
  • dist/v0.js
• The files appended in build-systems/single-pass.js@postProcessConcat (amp-date-picker and amp-viz-vega) show their network source.
  • Note that the max version of the above are compiled in the general gulp default when running the server
• Other observations:
  • dist/amp.js wasn't minified
  • dist/ww.js and dist/alp.js also don't connect to their Source Maps
• The Source Maps are created in gulpfile.js@compileJs

Suggestions

Check if reimplementing wrapMainBinaries using magic-string bundling functionality fixes it. Also check what's causing the issue with the others

[erwinmombay]

we'll need to look at prepend-global task too in build-system/tasks/prepend-global.js

[Marcial1234]

For readability purposes, I recommend refactoring build-systems/single-pass.js@wrapMainBinaries to:

function wrapMainBinaries() {
  const [prefix, suffix] = wrappers.mainBinary.split('<%= contents %>');
  // Cache the v0 file so we can prepend it to alternative binaries.
  const mainFile = 'dist/v0.js';
  const bootstrapCode = fs.readFileSync('dist/v0.js', 'utf8');
  jsFilesToWrap.forEach(file => {
    const path = `dist/${file}.js`;
    if (path === mainFile) {
      fs.writeFileSync(path, `${prefix}${bootstrapCode}${suffix}`);
    }
    else {
      const fileContent = fs.readFileSync(path).toString();
      const isAmpAltstring =  'self.IS_AMP_ALT=1;';
      const prependContent = `${isAmpAltstring}${prefix}${bootstrapCode}`;
      fs.writeFileSync(path, `${prependContent}${fileContent}${suffix}`);
    }
  });
}

Not making a PR for now since this code might be heavily modified to fix the current issue.

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @Marcial1234 Do you have any updates?

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @Marcial1234 Do you have any updates?

[rsimha]

@erwinmombay Still high priority? Assigning back to you so this doesn't get forgotten.

[rsimha]

Just went through the steps described in #19408 (comment) and was able to step through code with the cdn and local dev cases, for single and multi pass.

There have been several fixes to sourcemaps recently, so I'm marking this as resolved.