Use remapping to remap minified sourcemap into source code by jridgewell · Pull Request #37238 · ampproject/amphtml

jridgewell · 2021-12-16T19:19:12Z

For some reason, Terser is using the already changed property names instead of the original names (which exist inside the babel sourcemap). Instead of letting Terser do the remapping, we'll use our remapping library to remap our minified output maps into real source code.

samouri

Given we believe this to be a terser bug, lets file an issue

build-system/tasks/helpers.js

samouri · 2022-01-05T18:24:53Z

Testing this out locally, and I don't think this solves the issue. After doing more digging, I think I know the culprit of incorrect identifiers.

esbuild does not generate the names field: Support for sourcemap names evanw/esbuild#1296.
terser then generates an incorrect names field .
Our unminify code within the error-reporter-bot uses source-map.originalPositionFor which inspects the names field.

Potential fixes:

Make error bot more resilient by inspecting the original source and taking substrings instead of names.
Generate a correct names array in our build pipeline. To be clear, I still don't know why (2) is happening or why this PR doesn't fix it.

samouri · 2022-01-05T19:57:14Z

Discussed potential solution with @jridgewell.

Conclusion was to get each sourcemap separately. Then apply remapping to them all. Babel will produces 100s of sourcemaps, esbuild 1, and terser 1. Remapping can handle collating all of them.

We can remove this complication once esbuild supports outputting sourcemaps with names.

amp-owners-bot · 2022-01-06T00:45:58Z

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-transform-rename-privates/index.js

Hey @rsimha! These files were changed:

build-system/common/esbuild-babel.js
build-system/common/transform-cache.js

jridgewell

Ready for review.

jridgewell · 2022-01-06T02:43:32Z

build-system/babel-plugins/babel-plugin-transform-rename-privates/index.js

        return;
      }
-      key.replaceWith(t.identifier(`${name}AMP_PRIVATE_`));
+      key.replaceWith(t.inherits(t.identifier(`${name}AMP_PRIVATE_`), node));


When we replaced the node, it forgets what the original name is. We have to use t.inherits to pass the data from the original Identifier to the new Identifier.

jridgewell · 2022-01-06T02:45:10Z

build-system/common/transform-cache.js

-    transformPromise.then((contents) => fs.outputFile(filepath, contents));
+
+    const filepath = path.join(this.cacheDir, this.key_(hash));
+    transformPromise.then((contents) => fs.outputJson(filepath, contents));


This changes TransformCache to use JSON so that we can persist both the transformed code and the sourcemap in the cache directory.

build-system/tasks/helpers.js

Some build targets have directory parts in the `destFilename` as well as the `destDir`. We need the full dest directory path for remapping to work.

samouri

Nice work! I tested the fix, and sourcemaps lgtm. I checked both the names field as well as manually looked at the mapping in a visualizer.

samouri · 2022-01-06T02:41:25Z

build-system/babel-plugins/babel-plugin-transform-rename-privates/index.js

      }

-      if (!name.endsWith('_')) {
+      if (!name.endsWith('_') || name === '__proto__') {


FMI: how did you find the proto bug? Also how isn't this breaking prod? I'm assuming we only set proto to null ever?

I saw it in the names list as __proto___AMP_PRIVATE_

build-system/babel-plugins/babel-plugin-transform-rename-privates/index.js

samouri · 2022-01-06T02:57:14Z

build-system/common/esbuild-babel.js

 /**
 * Used to cache babel transforms done by esbuild.
- * @const {TransformCache}
+ * @type {TransformCache<!CacheMessageDef>}


We no longer need the !, since TS assumes nonnullable (this isn't checked via closure)

Suggested change

* @type {TransformCache<!CacheMessageDef>}

* @type {TransformCache<CacheMessageDef>}

one more thing! pretty sure you can add @readonly for variables meant to be const (docs)

Due to rebase issue, I'm going to fix this in a followup PR.

@readonly didn't actually work for me when I tested it out. Maybe only for class props

…nce-attr-to-hero-img * 'main' of github.com:ampproject/amphtml: (525 commits) mathml storybook: supply missing component definition. (#37326) storybook: Iframe --> BentoIframe (#37327) 🖍 [Story system layer] New ad badge (#37311) 🐛 [amp story] Replay/next page button bug fix (#37316) 🚀 [Story performance] Remove affiliate links (#37280) Compiler: Add amp-carousel-0.1 to the builder map (#37308) ⏪ [Story system layer] Audio icon disappears when story has background audio (#37314) 🚀 [Story performance] Remove story access (#37281) Fix remapping esbuild output on Windows (#37312) 🐛 adds in correct weight for amp-story-product-tag text (#37188) typechecking carousel: remove shame files (#37213) Use remapping to remap minified sourcemap into source code (#37238) SwG Release 0.1.22.199 (#37310) 🐛 Adds microsoft-edge protocol (#34168) Sync for validator cpp engine and cpp htmlparser (#37292) ✨ amp-story-shopping Updated currency with product price and correct Localized currency (#37249) ✨[Smartadserver ad extension] Implement support for Fast Fetch (#36991) Remove client-side-experiments-config.json from this repo (#37304) 🚮 Remove closure compiler logic from build-system. (#37296) 🌐 Added RTL ordering i18n for amp story shopping tag (#37252) ...

jridgewell requested a review from samouri December 16, 2021 19:19

samouri reviewed Dec 16, 2021

View reviewed changes

build-system/tasks/helpers.js Outdated Show resolved Hide resolved

build-system/tasks/helpers.js Show resolved Hide resolved

samouri reviewed Dec 16, 2021

View reviewed changes

build-system/tasks/helpers.js Outdated Show resolved Hide resolved

samouri mentioned this pull request Jan 4, 2022

🏗🚮 Clean up closure compiler code paths no longer used by amp dist #37173

Merged

jridgewell added 10 commits January 5, 2022 19:57

Use remapping to remap minified sourcemap into source code

ed4c823

Cleanup massaging

050e3f6

Eliminate sourcesContent at the esbuild level

a93a234

Fix undeclared variable

6811988

Store JSON messages in TransformCache

7356666

Extract babel sourcemaps during esbuild transformation

e73b597

Remap sourcemaps through terser -> esbuild -> babel

f7ab6c6

Remove TODO

0ebfcc5

Fix merge conflict

2b79c95

Preserve original key name

bac4002

jridgewell force-pushed the terser-sourcemap branch from f95639c to bac4002 Compare January 6, 2022 02:25

jridgewell commented Jan 6, 2022

View reviewed changes

Make sourceRoot include the full destFile

09dbdc2

Some build targets have directory parts in the `destFilename` as well as the `destDir`. We need the full dest directory path for remapping to work.

samouri approved these changes Jan 6, 2022

View reviewed changes

jridgewell merged commit 5f9e38d into ampproject:main Jan 6, 2022

jridgewell deleted the terser-sourcemap branch January 6, 2022 20:24

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jan 12, 2022

	* @type {TransformCache<!CacheMessageDef>}
	* @type {TransformCache<CacheMessageDef>}

Conversation

jridgewell commented Dec 16, 2021

Uh oh!

samouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samouri commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

jridgewell Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

jridgewell Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samouri left a comment

Choose a reason for hiding this comment

Uh oh!

samouri Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

jridgewell Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samouri Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

samouri Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

samouri Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

samouri commented Jan 5, 2022 •

edited

Loading

samouri commented Jan 5, 2022 •

edited

Loading

amp-owners-bot bot commented Jan 6, 2022 •

edited

Loading

samouri Jan 6, 2022 •

edited

Loading