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

Add support for React Native window location format Fixes sinonjs/sin… #1363

Merged
merged 5 commits into from
Apr 24, 2017
Merged

Add support for React Native window location format Fixes sinonjs/sin… #1363

merged 5 commits into from
Apr 24, 2017

Conversation

greena13
Copy link
Contributor

Purpose (TL;DR)

Fix issue #1362 and allows sinon to be used with React Native builds (where the location object is not stored directly on window).

Preserves existing behaviour for all other environments, including fallback object.

How to verify

  1. Check out this branch (see github instructions below)

  2. npm install

  3. import sinon into a React Native project and test on Android and iOS that it does not raise the error

    undefined is not an object (evaluating 'wloc.protocol')

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 95.334% when pulling 8f0e570 on greena13:master into f701abe on sinonjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 95.334% when pulling deee3dd on greena13:master into f701abe on sinonjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 95.334% when pulling 43b1768 on greena13:master into f701abe on sinonjs:master.

@coveralls
Copy link

coveralls commented Mar 24, 2017

Coverage Status

Coverage decreased (-0.04%) to 95.334% when pulling 77a360a on greena13:master into f701abe on sinonjs:master.

if (typeof window.location !== "undefined") {
// Browsers place location on window
return window.location;
} else if ((typeof window.window !== "undefined") && (typeof window.window.location !== "undefined")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, that is proper weird, but if this works ... then fine.

window.window.location? jeez 😸

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for reasons unknown to me, this does indeed seem to be where location is stored, in the versions of React Native that I tested against.

if ( typeof window !== "undefined") {
if (typeof window.location !== "undefined") {
// Browsers place location on window
return window.location;
Copy link
Member

Choose a reason for hiding this comment

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

When you use return inside if statements, you can avoid using else.

See http://rmurphey.com/blog/2012/12/10/js-conditionals

Also, you can unfold this code a bit, so there are fewer levels of indentation == easier to understand.

function getWindowLocation() {
    if (typeof window === "undefined") {
        // Fallback
        return getDefaultWindowLocation();
    }

    if (typeof window.location !== "undefined") {
        // Browsers place location on window
        return window.location;
    }

    if ((typeof window.window !== "undefined") && (typeof window.window.location !== "undefined")) {
        // React Native on Android places location on window.window
        return window.window.location;
    }

    return getDefaultWindowLocation();

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I agree, it can be simplified in this way.

@mroderick mroderick added the semver:patch changes will cause a new patch version label Apr 18, 2017
@coveralls
Copy link

coveralls commented Apr 22, 2017

Coverage Status

Coverage decreased (-0.02%) to 95.351% when pulling 4fe0a73 on greena13:master into f701abe on sinonjs:master.

@greena13
Copy link
Contributor Author

Patch suggestions applied. :)

@fatso83 fatso83 merged commit 0127365 into sinonjs:master Apr 24, 2017
@fatso83
Copy link
Contributor

fatso83 commented Apr 24, 2017

Thanks for your contributions!

@mroderick
Copy link
Member

sinon@2.2.0 is now on npm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch changes will cause a new patch version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants