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 url rewritting to new version #5669

Merged
merged 2 commits into from Oct 19, 2016

Conversation

jridgewell
Copy link
Contributor

We were rewriting requests like so:

  • /v0/amp-comp.js -> rtv/123/amp-comp.js
  • rtv/100/v0/amp-comp.js ->rtv/123/amp-comp.js

Notice that the rewrites have dropped the v0/ directory. Doh!

I've also added tests so that v1 requests will work (whenever we release one).

Fixes #5575.

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM with one comment.

expect(sw.rtvVersion(prefixless)).to.equal(`01${version}`);
describe('with RTV versioned file', () => {
const v0 = `https://cdn.ampproject.org/rtv/${rtv}/v0.js`;
const v1 = v0.replace('v0', 'v1');
Copy link
Member

Choose a reason for hiding this comment

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

Please hardcode the full strings. That is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion, done.

Makes things MUCH clearer.
@jridgewell jridgewell merged commit fdfd270 into ampproject:master Oct 19, 2016
@jridgewell jridgewell deleted the cache-sw-fix-url-rewrite branch October 19, 2016 19:51
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Fix url rewritting to new version

Fixes ampproject#5575.

* Use hardcoded urls

Makes things MUCH clearer.
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Fix url rewritting to new version

Fixes ampproject#5575.

* Use hardcoded urls

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

Successfully merging this pull request may close these issues.

None yet

3 participants