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

build-system: Support alternate sourcemap URLs #27661

Merged
merged 4 commits into from
Apr 10, 2020

Conversation

mdmower
Copy link
Contributor

@mdmower mdmower commented Apr 9, 2020

Support --sourcemap_url flag in gulp dist to customize the base URL
used in sourcemap links. The value passed to this flag should include a
placeholder {version} that will be replaced with the actual version.

For example:

gulp dist --sourcemap_url "https://example.com/{version}/root/" --version_override "12345"

would set the amp.js sourcemap URL to

https://example.com/12345/root/src/amp.js

This feature improves support for self-hosting the AMP runtime. It will be documented in the "Hosting the AMP framework" guide.

Support `sourcemap_url` flag in `gulp dist` to customize the base URL
used in sourcemap links. For example:

  gulp dist --sourcemap_url "https://example.com/{version}/root/"
@mdmower
Copy link
Contributor Author

mdmower commented Apr 9, 2020

@rsimha Could you help me understand an inconsistency I'm seeing between the sourcemap served from cdn.ampproject.org and that served from cdn.ampproject.org/rtv/<rtv>?

@rsimha
Copy link
Contributor

rsimha commented Apr 9, 2020

@rsimha Could you help me understand an inconsistency I'm seeing between the sourcemap served from cdn.ampproject.org and that served from cdn.ampproject.org/rtv/<rtv>?

I think this might be a known problem that @jridgewell and @rcebulko are in the process of fixing. Will let them chime in here.

@mdmower
Copy link
Contributor Author

mdmower commented Apr 9, 2020

@rsimha Could you help me understand an inconsistency I'm seeing between the sourcemap served from cdn.ampproject.org and that served from cdn.ampproject.org/rtv/<rtv>?

I think this might be a known problem that @jridgewell and @rcebulko are in the process of fixing. Will let them chime in here.

Thanks for deciphering my question. I accidentally hit shift+enter which posted my comment. Here's the discrepancy I'm seeing:

Using amp.js as a sample, sometimes there an extra /src in the URLs:

https://cdn.ampproject.org/rtv/012004041903580/v0.js.map --> https://raw.githubusercontent.com/ampproject/amphtml/2004041903580/src/src/amp.js

https://cdn.ampproject.org/rtv/012004030010070/v0.js.map --> https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/amp.js

https://cdn.ampproject.org/v0.js.map --> https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/amp.js

When I build locally, with gulp dist --type --version_override 2004030010070 --> https://raw.githubusercontent.com/ampproject/amphtml/2004030010070/src/src/amp.js

@kristoferbaxter
Copy link
Contributor

See #27665 for explanation of why this /src/src issue exists.

You'll want to pull in that PR's changes after it lands before continuing.

@jridgewell
Copy link
Contributor

I think this can actually be merged when ready, and I'll update my PR to incorporate it.

@mdmower
Copy link
Contributor Author

mdmower commented Apr 9, 2020

I think this can actually be merged when ready, and I'll update my PR to incorporate it.

@rsimha - Do you have a preference about the order in which the PRs are committed? Is the suggestion to use a placeholder {version} in the URL acceptable (see PR message)?

return `https://raw.githubusercontent.com/ampproject/amphtml/${internalRuntimeVersion}/`;
} else if (argv.sourcemap_url) {
return String(argv.sourcemap_url).replace(
'{version}',
Copy link
Contributor

@rsimha rsimha Apr 9, 2020

Choose a reason for hiding this comment

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

@rsimha - Do you have a preference about the order in which the PRs are committed? Is the suggestion to use a placeholder {version} in the URL acceptable (see PR message)?

@mdmower I assume this is the only place where the version passed in via the command line flag is substituted, correct?

Regarding the actual substitution string, we already use $internalRuntimeVersion$ in our source code:

/**
* Returns the internal AMP runtime version. Note that this is not the RTV,
* which is a prefix and the runtime version.
*
* The call sites for this function are replaced with a compile time constant
* string.
*
* @return {string}
*/
export function internalRuntimeVersion() {
return '$internalRuntimeVersion$';
}

Would it work if you used the same string for URL substitution as well?

gulp dist --sourcemap_url "https://example.com/$internalRuntimeVersion$/root/" --version_override "12345"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this is the only place where the version passed in via the command line flag is substituted, correct?

Correct.

we already use $internalRuntimeVersion$... Would it work if you used the same string for URL substitution as well?

Thanks for pointing that out! Sure, I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rsimha Actually, I'm questioning whether string $internalRuntimeVersion$ is the best choice for a user-facing flag. I've tried to make a consistent distinction between a version and a runtime version, reserving the latter to mean (config + version).

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I have no strong personal preference here, since it is a limited use case that you are more familiar with. (Perhaps $version$ is a good compromise?)

Once decided, we should mention the actual substitution string in the flag notes for --sourcemap_url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to keep {version} since shells would treat $version$ like an environment variable $version with an appended $ in common use cases like

gulp dist --sourcemap_url "https://example.com/$version$/"

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.

This PR LGTM, modulo the version substitution changes. Will let @jridgewell provide final approval, since he will need to incorporate this change into his sourcemaps fix.

@mdmower mdmower marked this pull request as ready for review April 9, 2020 22:46
@jridgewell
Copy link
Contributor

Merging so that I can rebase on it.

@jridgewell jridgewell merged commit e4100a4 into ampproject:master Apr 10, 2020
@mdmower mdmower deleted the pr-sourcemap branch April 11, 2020 18:12
zetagame pushed a commit to jwplayer/amphtml that referenced this pull request Apr 13, 2020
* build-system: Support alternate sourcemap URLs

Support `sourcemap_url` flag in `gulp dist` to customize the base URL
used in sourcemap links. For example:

  gulp dist --sourcemap_url "https://example.com/{version}/root/"

* Revisions from review

* Fix placeholder

* Fix placeholder
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

6 participants