-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Handle cases where self
isn't defined
#253
Conversation
Looks good! @dgraham Any objections? |
Invoking the wrapper function with |
Wouldn't this still throw a |
Good point @chrisirhc. Someone should test this out in an isomorphic environment and report back. I have no idea how it would handle the |
Hmm, yes that is true. Another option could be: this.self || this Which works exactly the same way but should not throw. |
9045eec
to
504e4a4
Compare
I have updated the code as per the above comment. |
Can someone test this out in an isomorphic environment, as per people's complaints on aforementioned threads? |
Node - master branch:
Node - this PR:
React Native - master I can't speak for other environments though |
@@ -378,4 +378,4 @@ | |||
}) | |||
} | |||
self.fetch.polyfill = true | |||
})(); | |||
})(this.self || this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I would like to revert to @chrisirhc's idea by doing
typeof self !== 'undefined' ? self : this
simply because that approach has been better battle-tested by compilers such as Browserify.
504e4a4
to
ad009dc
Compare
@mislav updated as-per your comment |
Handle cases where `self` isn't defined
Thank you! |
Hey @mislav did you forget to publish to npm, when I install 0.11.0 get the following:
|
@yorkie Something's wrong with our build system and auto-publish on npm. Stay tuned. |
I see, is there something I can help? This is no hurry, but if we can get a working notification with npm, that would be so sweet :-) |
I just did a manual `npm publish` for now.
|
It's working now, very thanks to you @mislav 👍 |
As per: #125 (comment)
Resolves #252, #238, #165, #125, #124, and probably a few others.
There are a lot of different ways this code could be written. If you have style preferences contrary to the way I've done this, I'd be very happy to change it.