Removed the dead getViewerUrl function
#17404
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
torch2424
force-pushed
the
getViewerUrl-15425
branch
from
efb9496
to
88c32cb
Compare
torch2424
force-pushed
the
getViewerUrl-15425
branch
from
88c32cb
to
a8561a0
Compare
|
CLAs look good, thanks! |
test/functional/test-viewer.js
Outdated
| @@ -1499,135 +1499,6 @@ describe('Viewer', () => { | |||
| const viewer = new Viewer(ampdoc); | |||
| expect(viewer.getResolvedViewerUrl()).to.equal('https://acme.org/doc1'); | |||
| }); | |||
|
|
|||
| it('should always return current location for top-level window', () => { | |||
There was a problem hiding this comment.
It'd seem some of these tests are still valid...
There was a problem hiding this comment.
Yes, but I figured there may be similar tests for just the viewer.getResolvedViewerUrl(), since these tests appear to mostly be testing viewer.getViewerUrl(). Would you like me to remove calls to viewer.getViewerUrl() and replace with viewer.getResolvedViewerUrl() to get these tests running and valid?
My apologies, I just don't have much context on how/where this feature is used, and how the tests relate to the separate calls.
There was a problem hiding this comment.
Good question. I think you can just replace isTrustedViewer() in the tests and only test the getResolvedViewerUrl()?
There was a problem hiding this comment.
So Implemented this change in a local commit, and noticed the following code within the this.viewerUrl_ promise: https://github.com/ampproject/amphtml/blob/master/src/service/viewer-impl.js#L425
Meaning, the resolved URL is not updated without calling the promise that is returned by viewer.getViewerUrl() which is simply this.viewerUrl_.
I tried replacing with isTrustedViewer(), but that wont update the this.resolvedViewerUrl_, therefore the tests will break because the viewer url never actually updates.
Perhaps this code is not entirely dead as mentioned in #15425 ? Or we need to update the viewer-impl.js accordingly to update it's resolved url.
There was a problem hiding this comment.
Yeah. Please consult @lannka if that code is indeed dead. Obviously some part of it is still in use.
src/service/viewer-impl.js
Outdated
| * @return {!Promise<string>} | ||
| */ | ||
| getViewerUrl() { | ||
| return this.viewerUrl_; |
There was a problem hiding this comment.
Also, what about this variable itself? Should it be removed as well?
|
cc @dvoytenko After talking with @lannka we came to the conclusion that the code was only used by tests, but were an important part of testing overriding viewer urls. Thus we marked the function as visible for testing. Though, we feel like the file should still be refactored. |
src/service/viewer-impl.js
Outdated
| @@ -890,7 +891,12 @@ export class Viewer { | |||
| isTrustedViewerOrigin_(urlString) { | |||
| /** @const {!Location} */ | |||
| const url = parseUrlDeprecated(urlString); | |||
| if (url.protocol != 'https:') { | |||
| const {protocol} = url; | |||
|
@dvoytenko Whenever you get the chance can you approve and merge this in? It became a 1 line fix in the end. 馃槃 |
* Simply removed the dead `getViewerUrl` function * Removed tests with calls to getViewerUrl * Marked the getViewerUrl at visible for testing * Removed som random code that got added by syncing with master * Removed more random code that got added by the sync
closes #15425
馃毊 Simply removes the unused function
getViewerUrl. This is not used anywhere as mentioned in the issue.However, this also removes tests found in
test/functional/test-viewer.jsfor this function, which may/may not be desired. Let me know in the comments below.