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

🏗 Fix sourcemap sources paths #27665

Merged
merged 4 commits into from Apr 13, 2020

Conversation

jridgewell
Copy link
Contributor

This fixes the sourcemaps' sources paths. Closure treats each source location as being relative to the sourcemap. Unlike regular gulp plugins, Closure pretends the sourcemap lives in the same directory as the compiled file. So, when resolving, it was adding the directory path twice.

And in single pass, we actually never had working sourcemap paths. The paths we always /src/*, and not the raw.githubusercontent.com they should have been.

This also removes a now-unneeded root this replacement, which was being done with gulp-regexp-sourcemaps. For some reason, this removes all of the source paths. Thankfully, it's no longer needed as Closure's getGlobal has been updated to https://github.com/google/closure-compiler/blob/v20200315/src/com/google/javascript/jscomp/js/util/global.js.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 9, 2020

Hey @erwinmombay, these files were changed:

build-system/compile/single-pass.js

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Not familiar enough with some of the sourcemap libraries/processes to have 100% confidence, but it all looks reasonable enough and if it works, it works 😃 Thanks for fixing this so quickly!

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Minor comments, but looks good.

Thanks for cleaning up the remapping usage too!

package.json Show resolved Hide resolved
build-system/compile/single-pass.js Outdated Show resolved Hide resolved
build-system/compile/closure-compile.js Outdated Show resolved Hide resolved
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Travis check found this.

build-system/compile/post-closure-babel.js Show resolved Hide resolved
@rcebulko
Copy link
Contributor

rcebulko commented Apr 9, 2020

Because of the duplicated paths, errors being reported right now aren't being grouped properly, making it hard to tell what's a new error and what isn't. When this is finalized and approved, it may be worth cherry-picking into at least Beta/Experimental

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Very excited about this PR!

I'm adding a blocking review because I've requested some build infrastructure changes that will simplify our release code.

build-system/compile/closure-compile.js Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/compile/single-pass.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
build-system/compile/closure-compile.js Show resolved Hide resolved
build-system/compile/compile.js Outdated Show resolved Hide resolved
build-system/compile/compile.js Show resolved Hide resolved
build-system/compile/compile.js Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

👏👏👏

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Awesome work! Looks great and cleans up quite a bit of cruft.

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.

None yet

8 participants