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

✨Accept canonical URL in proxy form #21043

Merged
merged 3 commits into from
Feb 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion build-system/app-index/main.css
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,12 @@ a.underlined::before {
margin: 20px 0 0;
font-size: 12px;
text-align: right;
color: #888;
}
.form-info a {
.form-info > span + a {
margin-left: 8px;
}
.form-info > a {
font-weight: bold;
}
header {
Expand Down
1 change: 1 addition & 0 deletions build-system/app-index/proxy-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = () => html`<div class="block">
pattern="^(https?://)?[^\\s]+$" />
</label>
<div class="form-info">
<span>Takes canonical, AMPHTML and Google viewer URLs.</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for adding the hint to the UI 😄 👍

<a href="https://github.com/ampproject/amphtml/blob/master/contributing/TESTING.md#document-proxy"
target="_blank">
What's this?
Expand Down
59 changes: 52 additions & 7 deletions build-system/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,19 +119,64 @@ app.get('/serve_mode_change', (req, res) => {
// - /proxy/?url=hello.com?mode=/shadow/ 👉 /shadow/proxy/s/hello.com
// - /proxy/?url=https://hello.com 👉 /proxy/s/hello.com
// - /proxy/?url=https://www.google.com/amp/s/hello.com 👉 /proxy/s/hello.com
// - /proxy/?url=hello.com/canonical 👉 /proxy/s/hello.com/amp
//
// This passthrough is useful to generate the URL from <form> values,
// (See ./app-index/proxy-fom.js)
app.get('/proxy', (req, res) => {
// (See ./app-index/proxy-form.js)
app.get('/proxy', async(req, res, next) => {
const {mode, url} = req.query;
const prefix = (mode || '').replace(/\/$/, '');
const sufixClearPrefixReStr =
'^http(s?)://' +
const urlSuffixClearPrefixReStr =
'^https?://' +
'((www\.)?google\.(com?|[a-z]{2}|com?\.[a-z]{2}|cat)/amp/s/)?';
const sufix = url.replace(new RegExp(sufixClearPrefixReStr, 'i'), '');
res.redirect(`${prefix}/proxy/s/${sufix}`);
const urlSuffix = url.replace(new RegExp(urlSuffixClearPrefixReStr, 'i'), '');

try {
const ampdocUrl = await requestAmphtmlDocUrl(urlSuffix);
const ampdocUrlSuffix = ampdocUrl.replace(/^https?:\/\//, '');
const modePrefix = (mode || '').replace(/\/$/, '');
const proxyUrl = `${modePrefix}/proxy/s/${ampdocUrlSuffix}`;
res.redirect(proxyUrl);
} catch ({message}) {
console.log(`ERROR: ${message}`);
next();
}
});


/**
* Resolves an AMPHTML URL from a canonical URL. If AMPHTML is canonical, same
* URL is returned.
* @param {string} urlSuffix URL without protocol or google.com/amp/s/...
* @param {string=} protocol 'https' or 'http'. 'https' retries using 'http'.
* @return {!Promise<string>}
*/
function requestAmphtmlDocUrl(urlSuffix, protocol = 'https') {
const defaultUrl = `${protocol}://${urlSuffix}`;
console.log(`Fetching URL: ${defaultUrl}`);
return new Promise((resolve, reject) => {
request(defaultUrl, (error, response, body) => {
if (error || (response && (
response.statusCode < 200 || response.statusCode >= 300))) {

if (protocol == 'https') {
return requestAmphtmlDocUrl(urlSuffix, 'http');
}
return reject(new Error(error || `Status: ${response.statusCode}`));
}
const {window} = new jsdom.JSDOM(body);
const linkRelAmphtml = window.document.querySelector('link[rel=amphtml]');
if (!linkRelAmphtml) {
return resolve(defaultUrl);
}
const amphtmlUrl = linkRelAmphtml.getAttribute('href');
if (!amphtmlUrl) {
return resolve(defaultUrl);
}
return resolve(amphtmlUrl);
});
});
}

/*
* Intercept Recaptcha frame for,
* integration tests. Using this to mock
Expand Down
10 changes: 6 additions & 4 deletions contributing/TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,9 @@ AMP ships with a local proxy for testing production AMP documents with the local

For any public AMP document like: `http://output.jsbin.com/pegizoq/quiet`,

You can access it with the local JS at
You can access it with the local JS by using the form in
[`http://localhost:8000`](http://localhost:8000) or by accessing the proxy URL
directly:

`http://localhost:8000/proxy/output.jsbin.com/pegizoq/quiet`.

Expand Down Expand Up @@ -327,13 +329,13 @@ If you are only testing a single file, you can use `gulp firebase --file=path/to

## End-to-End Tests

You can run and create E2E tests locally during development. Currently tests only run on Chrome, but support for additional browsers is underway. These tests have not been added to our CI build yet - but they will be added soon.
You can run and create E2E tests locally during development. Currently tests only run on Chrome, but support for additional browsers is underway. These tests have not been added to our CI build yet - but they will be added soon.

Run all tests with:
Run all tests with:
```
gulp e2e
```

The task will kick off `gulp build` and then `gulp serve` before running the tests. To skip building the runtime, use `--nobuild`.

Create your own tests by adding them to `test\e2e` or `extensions\**\test-e2e`. For examples, see `test\e2e\test-github.js` and `extensions\amp-carousel\0.2\test-e2e\test-carousel.js`.
Create your own tests by adding them to `test\e2e` or `extensions\**\test-e2e`. For examples, see `test\e2e\test-github.js` and `extensions\amp-carousel\0.2\test-e2e\test-carousel.js`.