Enhance location detection within utils #2167
Conversation
send 'iv' param if present and full url in `site.page`
Enhance location detection within utils
There was a problem hiding this comment.
@rachelrj Thanks for the PR. Adapter changes look good, a few comments regarding the changes to utils for your review
src/utils.js
Outdated
|
|
||
| return location; | ||
| const parseFullUrl = function(locString) { |
There was a problem hiding this comment.
There's a parse function in the src/url.js module that parses a url and returns a similar looking object, would reusing that function in place of this one fit this use case?
There was a problem hiding this comment.
Yes, though I added the href into the return object of that function.
src/utils.js
Outdated
| @@ -157,17 +157,50 @@ export function parseGPTSingleSizeArray(singleSize) { | |||
| } | |||
| }; | |||
|
|
|||
| exports.getTopWindowLocation = function () { | |||
| let location; | |||
| export function getTopWindowLocation() { | |||
There was a problem hiding this comment.
Several modules rely on this function, changing it may introduce subtle differences. Any reason to not keep the existing getTopWindowLocation as is, and create a new function for this case?
There was a problem hiding this comment.
The old function getTopWindowLocation either returns window.top.location or returns window.location.
The new getTopWindowLocation returns window.location, window.top.location, window.document.referrer, window.document.location.ancestorOrigins[0], or an object that recreates the values within window.location
The new version attempts to get the same info, just from additional objects.
I checked the modules, and I think this new version should fit all the current use cases.
I am not opposed to creating a new function if you’re worried about it.
There was a problem hiding this comment.
Ok, thanks for the explanation. Changes to core require a second review, assigning that now
src/utils.js
Outdated
| try { | ||
| // force an exception in x-domain enviornments. #1509 | ||
| window.top.location.toString(); |
There was a problem hiding this comment.
Given the comment, not sure removing this is the correct thing to do, unless there's an equivalent fix in the new code i'm not seeing?
There was a problem hiding this comment.
Yeah, I didn't realize that the exception wasn't thrown when trying to access window.top until I read that 1509 PR. Interesting. Thanks for pointing out. I added back in.
CR for enhanced-location-detection PR
src/utils.js
Outdated
| do { | ||
| currentWindow = currentWindow ? currentWindow.parent : window; | ||
| if (currentWindow.document && currentWindow.document.referrer) { | ||
| referrerLoc = currentWindow.document.referrer; |
There was a problem hiding this comment.
won't this return a string when an object is expected?
There was a problem hiding this comment.
Returns a string to getIframeParentLoc which then gets sent to parse which turns the string into the expected object.
|
I'll work on adding some unit tests. |
Reorg gettopwindowlocation util & add tests.
|
I reorganized the additions to Also to use |
|
There really are not any elegant solutions that I've seen to mock window and the like. The other option is to pass |
|
@rachelrj |
|
Fixed merge conflict. |
Type of change
Description of change
Check other window objects for location if cannot find.