Skip to content

🐛 Rewrite relative urls in the bookend#26490

Merged
Enriqe merged 4 commits intoampproject:masterfrom
Enriqe:bookend-urls2
Jan 28, 2020
Merged

🐛 Rewrite relative urls in the bookend#26490
Enriqe merged 4 commits intoampproject:masterfrom
Enriqe:bookend-urls2

Conversation

@Enriqe
Copy link
Copy Markdown
Contributor

@Enriqe Enriqe commented Jan 24, 2020

Closes #26316

Resolves relative URLs specified in the bookend configuration. See linked issue for more details.

  • Note: I know we don't like to write code that is only used in tests but this was the best I could do given that Sinon doesn't like stubbing document.location / window.location and I've seen other tests do similar things.

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jan 24, 2020

Hey @gmajoulet, these files were changed:

  • extensions/amp-story/1.0/bookend/components/article.js
  • extensions/amp-story/1.0/bookend/components/cta-link.js
  • extensions/amp-story/1.0/bookend/components/landscape.js
  • extensions/amp-story/1.0/bookend/components/portrait.js
  • extensions/amp-story/1.0/test/test-amp-story-bookend.js
  • extensions/amp-story/1.0/utils.js

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/bookend/components/article.js
  • extensions/amp-story/1.0/bookend/components/cta-link.js
  • extensions/amp-story/1.0/bookend/components/landscape.js
  • extensions/amp-story/1.0/bookend/components/portrait.js
  • extensions/amp-story/1.0/test/test-amp-story-bookend.js
  • extensions/amp-story/1.0/utils.js

@Enriqe Enriqe requested review from gmajoulet and newmuis January 24, 2020 23:13
gmajoulet
gmajoulet reviewed Jan 27, 2020
View reviewed changes
extensions/amp-story/1.0/test/test-amp-story-bookend.js Outdated
};

storyUtils.getDocLocation.restore();
const fakeDoc = {
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Jan 27, 2020

Choose a reason for hiding this comment

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

Can you try using one of these methods to mock the location object? I've found three ways to do it that'd yield easier code and much better test coverage. By order of personal preference:

  1. amphtml/extensions/amp-install-serviceworker/0.1/test/test-amp-install-serviceworker.js

    Lines 553 to 555 in 244d647

    win.location.resetHref(
    'https://cdn.ampproject.org/c/s/example.com/doc1'
    );

  2. amphtml/extensions/amp-analytics/0.1/test/test-cookie-writer.js

    Line 273 in 244d647

    win.location = 'https://www.example.com/?a=123&b=567';

  3. amphtml/extensions/amp-access-laterpay/0.1/test/test-amp-access-laterpay.js

    Line 25 in 244d647

    location: 'https://pub.com/doc1',

Copy link
Copy Markdown
Contributor Author

@Enriqe Enriqe Jan 27, 2020

Choose a reason for hiding this comment

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

Whenever we get the location in the bookend, we do it by calling document.location. Mocking the document object properties in the tests is not as straightforward since it is an iframe inside a shadow dom created by Sinon.

We could use the window object instead of the document in the bookend code, do you prefer it this way?

gmajoulet
gmajoulet reviewed Jan 27, 2020
View reviewed changes
extensions/amp-story/1.0/test/test-amp-story-bookend.js Outdated
'amp-story-bookend',
{
win: {
location:
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Jan 27, 2020

Choose a reason for hiding this comment

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

Can we do something like this instead? It's almost impossible to review this change otherwise :(

const location = 'foo';
describes.realWin('amp-story-bookend', {location, amp: true}, env => {

Copy link
Copy Markdown
Contributor Author

@Enriqe Enriqe Jan 27, 2020

Choose a reason for hiding this comment

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

Sure, just note that we have to use fakeWin in order to use win.location.resetHref

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Jan 27, 2020

PTAL @gmajoulet

gmajoulet
gmajoulet approved these changes Jan 27, 2020
View reviewed changes
gmajoulet
gmajoulet reviewed Jan 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

What caused this massive diff? :(

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Jan 28, 2020 •
edited
Loading

I grouped semantically similar tests into separate describe, hopefully making it easier to read and review these in the future. The tests themselves didn't change from the last commit.

describe('analytics', () => {...})
describe('portrait component', () => {...})
describe('landscape component', () => {...})
describe('article component', () => {...})

@Enriqe Enriqe merged commit 5dae099 into ampproject:master Jan 28, 2020
@Enriqe Enriqe deleted the bookend-urls2 branch January 28, 2020 18:23
westonruter added a commit to westonruter/amphtml that referenced this pull request Jan 28, 2020
@westonruter
Merge branch 'master' of github.com:ampproject/amphtml into add/amp-i…
2b6e016 
…frame-pymjs-support

* 'master' of github.com:ampproject/amphtml: (436 commits)
  🐛Add computed styles to fake win (ampproject#26514)
  📦 Update dependency eslint-config-prettier to v6.10.0 (ampproject#26518)
  🐛 Rewrite relative urls in the bookend (ampproject#26490)
  ✨ [amp-list] Introduce "amp-state:" as a usable protocol for the src attribute. (ampproject#26284)
  Add terminal newline to files.txt (ampproject#26513)
  📦 Update dependency chromedriver to v79.0.2 (ampproject#26515)
  ♻️ Change the opt-in cookie values to reflect upcoming CDN changes (ampproject#26489)
  ♻️<amp-next-page> v2 minor renaming and fixes (ampproject#26468)
  fix link (ampproject#26508)
  amp-list: fix broken test re. missing layout (ampproject#26509)
  performance: emit timeOrigin w/ polyfill (ampproject#26485)
  Position n+1 story desktop page before positioning attributes are set. (ampproject#26488)
  ✨amp-nested-menu: allow svg into (ampproject#26502)
  Log user warning when missing URL arg for shadow doc (ampproject#26290)
  📦 Update dependency puppeteer to v2.1.0 (ampproject#26501)
  Ensure that fluid slots hidden by media queries do not issue ad requests. (ampproject#26352)
  📦 Update dependency mocha to v7.0.1 (ampproject#26495)
  📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491)
  Revert 'Move mutator implementations out to a standalone service' (ampproject#26479)
  Fix syntax error (ampproject#26481)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@newmuis newmuis Awaiting requested review from newmuis

Assignees

No one assigned

Labels

cla: yes PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bookend relative links should be resolved when served from an AMP Cache

4 participants

@Enriqe @gmajoulet @googlebot @ampprojectbot