Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weā€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

šŸ› Insert empty newline as first line of minified outputs #37640

Merged
merged 9 commits into from
Feb 11, 2022
2 changes: 1 addition & 1 deletion build-system/babel-config/minified-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ function getMinifiedConfig() {
return {
compact: false,
plugins,
sourceMaps: 'both',
sourceMaps: true,
presets: [presetTypescript, presetEnv],
retainLines: true,
assumptions: {
Expand Down
2 changes: 1 addition & 1 deletion build-system/babel-config/unminified-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function getUnminifiedConfig() {
compact: false,
plugins: unminifiedPlugins,
presets: unminifiedPresets,
sourceMaps: 'both',
sourceMaps: true,
assumptions: {
constantSuper: true,
noClassCalls: true,
Expand Down
104 changes: 100 additions & 4 deletions build-system/common/esbuild-babel.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@ const babel = require('@babel/core');
const path = require('path');
const {debug} = require('../compile/debug-compilation-lifecycle');
const {TransformCache, batchedRead, md5} = require('./transform-cache');
const Remapping = require('@ampproject/remapping');

/** @type {Remapping.default} */
const remapping = /** @type {*} */ (Remapping);

const argv = require('minimist')(process.argv.slice(2));

/**
* @typedef {{
* filename: string,
* code: string,
* map: Object,
* }}
*/
let CacheMessageDef;
Expand All @@ -33,6 +40,8 @@ function getEsbuildBabelPlugin(
enableCache,
{preSetup = () => {}, postLoad = () => {}} = {}
) {
const babelMaps = new Map();

/**
* @param {string} filename
* @param {string} contents
Expand All @@ -57,14 +66,14 @@ function getEsbuildBabelPlugin(
.then((result) => {
const {code, map} = /** @type {!babel.BabelFileResult} */ (result);
debug('post-babel', filename, code, map);
return {filename, code: code || ''};
return {filename, code: code || '', map};
});

if (enableCache) {
transformCache.set(hash, promise);
}

return promise.finally(postLoad);
return promise;
}

return {
Expand All @@ -73,12 +82,21 @@ function getEsbuildBabelPlugin(
async setup(build) {
preSetup();

const {initialOptions} = build;
const {sourcemap} = initialOptions;
const inlineSourcemap = sourcemap === 'inline' || sourcemap === 'both';
if (inlineSourcemap) {
initialOptions.sorucemap = true;
}

build.onLoad(
{filter: /\.(cjs|mjs|js|jsx|ts|tsx)$/, namespace: ''},
async (file) => {
const filename = file.path;
const babelOptions =
babel.loadOptions({caller: {name: callerName}, filename}) || {};
const babelOptions = /** @type {*} */ (
babel.loadOptions({caller: {name: callerName}, filename}) || {}
);
babelOptions.sourceMaps = true;

const {contents, hash} = await batchedRead(filename);
const rehash = md5(
Expand All @@ -97,11 +115,89 @@ function getEsbuildBabelPlugin(
rehash,
getFileBabelOptions(babelOptions, filename)
);

babelMaps.set(filename, transformed.map);
postLoad?.();
return {contents: transformed.code};
}
);

build.onEnd(async (result) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FMI: can you explain this refactor to onEnd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 8c829df, I changed Babel to output an inline sourcemap for esbuild to ingest, thinking it would remap correctly. But esbuild doesn't carry through names (which is why we switched to the complicated remapping + babelMaps in the first place). I'm correcting it this time, but I'm making the code local to the esbuild-babel plugin instead of spreading it across the plugin and consumer. Now the consumer just gets the correctly remapped sourcemaps.

const {outputFiles} = result;
const code = outputFiles.find(({path}) => !path.endsWith('.map'));
const map = outputFiles.find(({path}) => path.endsWith('.map'));

if (!map) {
return;
}

const root = path.dirname(map.path);
const remapped = remapping(
map.text,
(f, ctx) => {
// The Babel tranformed file and the original file have the same
// path, which makes it difficult to distinguish during remapping's
// load phase. To prevent an infinite recursion, we check if the
// importer is ourselves (which is nonsensical) and early exit.
if (f === ctx.importer) {
return null;
}

const file = path.join(root, f);
const map = babelMaps.get(file);
if (!map) {
if (file.includes('/node_modules/')) {
// Excuse node_modules since they may have been marked external
// (and so not processed by babel).
return null;
}
throw new Error(`failed to find sourcemap for babel file "${f}"`);
}
return map;
},
!argv.full_sourcemaps
);

const sourcemapJson = remapped.toString();
replaceOutputFile(outputFiles, map, sourcemapJson);
if (inlineSourcemap) {
const base64 = Buffer.from(sourcemapJson).toString('base64');
replaceOutputFile(
outputFiles,
code,
code.text.replace(
/sourceMappingURL=.*/,
`sourceMappingURL=data:application/json;charset=utf-8;base64,${base64}`
)
);
}
});
},
};
}

/**
* @param {Array<import('esbuild').OutputFile>} outputFiles
* @param {import('esbuild').OutputFile} original
* @param {string} text
*/
function replaceOutputFile(outputFiles, original, text) {
const index = outputFiles.indexOf(original);
if (index === -1) {
throw new Error(`couldn't find outputFile ${original.path}`);
}

let contents;
const file = {
path: original.path,
text,
get contents() {
// eslint-disable-next-line local/no-forbidden-terms
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add this as an exception in build-system/test-configs/forbidden-terms.js instead of an explicit eslint-disable here?

const te = new TextEncoder();
return (contents ||= te.encode(text));
},
};
outputFiles[index] = file;
}

const CJS_TRANSFORMS = new Set([
Expand Down
2 changes: 1 addition & 1 deletion build-system/tasks/check-sourcemaps.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ function checkSourcemapMappings(sourcemapJson, map) {
}

// See https://www.npmjs.com/package/sourcemap-codec#usage.
const firstLineMapping = decode(sourcemapJson.mappings)[0][0];
const firstLineMapping = decode(sourcemapJson.mappings)[1][0];
const [, sourceIndex = 0, sourceCodeLine = 0, sourceCodeColumn] =
firstLineMapping;

Expand Down
23 changes: 12 additions & 11 deletions build-system/tasks/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,11 +236,7 @@ function maybeToNpmEsmName(name) {
* @param {string} destFilename
*/
function handleBundleError(err, continueOnError, destFilename) {
let message = err.toString();
if (err.stack) {
// Drop the node_modules call stack, which begins with ' at'.
message = err.stack.replace(/ at[^]*/, '').trim();
}
const message = err.stack || err.toString();
log(red('ERROR:'), message, '\n');
const reasonMessage = `Could not compile ${cyan(destFilename)}`;
if (continueOnError) {
Expand Down Expand Up @@ -347,7 +343,6 @@ async function esbuildCompile(srcDir, srcFilename, destDir, options) {
entryPoints: [entryPoint],
bundle: true,
sourcemap: 'external',
sourceRoot: path.dirname(destFile),
sourcesContent: !!argv.full_sourcemaps,
outfile: destFile,
define: experimentDefines,
Expand Down Expand Up @@ -375,7 +370,7 @@ async function esbuildCompile(srcDir, srcFilename, destDir, options) {
const {outputFiles} = result;

let code = outputFiles.find(({path}) => !path.endsWith('.map')).text;
let map = JSON.parse(
const map = JSON.parse(
result.outputFiles.find(({path}) => path.endsWith('.map')).text
);
const mapChain = [map];
Expand All @@ -402,12 +397,13 @@ async function esbuildCompile(srcDir, srcFilename, destDir, options) {
code = result.code;
mapChain.unshift(result.map);
}
map = massageSourcemaps(mapChain, options);

const sourceMapComment = `\n//# sourceMappingURL=${destFilename}.map`;
await Promise.all([
fs.outputFile(destFile, `${code}\n${sourceMapComment}`),
fs.outputJson(`${destFile}.map`, map),
fs.outputFile(
destFile,
`${code}\n//# sourceMappingURL=${destFilename}.map`
),
fs.outputJson(`${destFile}.map`, massageSourcemaps(mapChain, options)),
]);

await finishBundle(destDir, destFilename, options, startTime);
Expand Down Expand Up @@ -534,6 +530,11 @@ async function minify(code, options = {}) {
output: {
beautify: !!argv.pretty_print,
keep_quoted_props: true,
// The AMP Cache will prepend content on the first line during serving
// (for AMP_CONFIG and AMP_EXP). In order for these to not affect
// the sourcemap, we must ensure there is no other content on the first
// line. If you remove this you will annoy Justin. Don't do it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

šŸ˜¢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for you. šŸ˜œ

preamble: ';',
},
sourceMap: true,
toplevel: true,
Expand Down
7 changes: 6 additions & 1 deletion build-system/tasks/release/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ async function compileDistFlavors_(flavorType, command, tempDir) {
if (argv.esm) {
command += ' --esm';
}
if (argv.full_sourcemaps) {
command += ' --full_sourcemaps';
}
log('Compiling flavor', green(flavorType), 'using', cyan(command));

execOrDie('amp clean --exclude release');
Expand Down Expand Up @@ -483,7 +486,8 @@ async function prependConfig_(outputDir) {
...target.config,
});

const contents = await fs.readFile(targetPath, 'utf-8');
const contents = await fs.readFile(targetPath, 'utf8');

return fs.writeFile(
targetPath,
`self.AMP_CONFIG=${channelConfig};/*AMP_CONFIG*/${contents}`
Expand Down Expand Up @@ -590,6 +594,7 @@ release.flags = {
'flavor':
'Limit this release build to a single flavor (can be used to split the release work across multiple build machines)',
'esm': 'Compile with --esm if true, without --esm if false or unspecified',
'full_sourcemaps': 'Include source code content in sourcemaps',
'dedup_v0':
'Removes duplicate copies of the v0/ subdirectory when they are the same files as those in the Stable (01-prefixed) channel',
};
7 changes: 7 additions & 0 deletions build-system/tasks/sourcemaps.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ function massageSourcemaps(mapChain, options) {
if (map.file) {
map.file = path.basename(map.file);
}
map.sources = map.sources.map((s) => {
if (s?.startsWith('../')) {
return s.slice('../'.length);
}
return s;
});

return map;
}

Expand Down
58 changes: 47 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.