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

🏗✨ Add a test for sourcemaps generation during minified compilation #27795

Merged
merged 5 commits into from
Apr 24, 2020
Merged

🏗✨ Add a test for sourcemaps generation during minified compilation #27795

merged 5 commits into from
Apr 24, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Apr 16, 2020

Background:

During minified compilation, we generate a sourcemap file called v0.js.map in addition to the runtime file v0.js. Sourcemap generation frequently breaks due to untested compilation toolchain changes. See #27681.

PR highlights:

  • Adds a task called gulp check-sourcemaps
  • Compiles the runtime with sourcemaps (gulp dist --core_runtime_only --full_sourcemaps)
  • Verifies the the sourceRoot field exists, and is of the correct format
  • Verifies that the sources array exists, and all paths in it point to valid files
  • Verifies that the mappings object exists, decodes it, and verifies that a mapping exists for a known sentinel piece of code

Usage:

# Build core runtime and check generated sourcemaps
gulp check-sourcemaps

# Check sourcemaps from previous invocation of gulp dist --full_sourcemaps
gulp check-sourcemaps --nobuild

Passing example:

image

Failing example (incorrect sourceRoot: wrong sourcemap domain):

image

Failing example (incorrect sources: revert fix in #27665):

image

Failing example (incorrect mappings: modify sentinel piece of code):

image

Fixes #27681

@jridgewell
Copy link
Contributor

Looking at the source code, source-map-unpacker doesn't actually regenerate the inputs from the sourcemap. It's just printing the included sourcesContent to a file. We need actual tests of the sourcema's mappings.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 16, 2020

Looking at the source code , source-map-unpacker doesn't actually regenerate the inputs from the sourcemap. It's just printing the included sourcesContent to a file.

Pretty sure this is not true. I made a known change to the source code in v0.js.map and it showed up as a diff. From this line, I'm pretty certain js.sourcesContent[idx] is coming from the map file, not from the original source.

Edit: Unless I misunderstood the original intent of a sourcemap file? Is the sourcesContent not what should be checked?

@rsimha rsimha marked this pull request as ready for review April 16, 2020 03:39
@amp-owners-bot
Copy link

Hey @estherkim, these files were changed:

build-system/pr-check/checks.js

@rsimha
Copy link
Contributor Author

rsimha commented Apr 16, 2020

@jridgewell I've added a step that compares ASTs of src/amp.js and dist/extracted/src/amp.js. There are diffs which you will see in these logs.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 21, 2020

As discussed offline with @jridgewell, we're still in search of an effective way to extract the mappings from v0.js.map and ensure their correctness (via say, a sentinel value). Until then, there's still some value in making sure the sourcemap file is generated, and the correct sourcemap URL is present.

I've modified the scope of this PR to add the gulp check-sourcemaps task and verify the sourcemaps URL. The rest will be done in a follow up PR when time permits.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 22, 2020

@choumx @jridgewell All changes made. This is now ready for review and merge.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2020

Added a new check for all the paths in the sources array.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 24, 2020

Added yet another check that implements #27681 (comment) and inspects the mappings field for a known sentinel value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Sourcemap "golden" tests
5 participants