-
Notifications
You must be signed in to change notification settings - Fork 224
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
Check for process object #79
Conversation
Can you base your PRs on master to be able to merge them independently? |
If you pull them in order you will be able to merge them independently. Unfortunately all three have conflicts which is way I opted to do it like this. |
Actually, don't pull this one yet! I think that there is a problem we haven't counted with... I think that browserify shims Whatever the reason, the tests fails in the browser for this one, so just hold on for a bit. |
Of course I can pull them in order but it defeat the goal of having separate PRs. The browserify test also fails in master, I have to figure out why. |
And I do not think it is a problem that browserify shims |
You are right, I tested in Safari instead of Chrome 👊 😵 The tests passes. The browserify test won't work until a bug in browserify is resolved, see #66 I understand that it defeats the pull request but if I had did it the other way, then whenever you merged one, the other one would have changed to "unable to automatically merge" because of merge conflicts. So I opted to enforce the order in which they are merged, I hope it's okay :) |
306a1d3
to
bdb900f
Compare
Rebased on master |
@@ -21,6 +21,10 @@ function isInBrowser() { | |||
return ((typeof window !== 'undefined') && (typeof XMLHttpRequest === 'function')); | |||
} | |||
|
|||
function hasGlobalProcess() { |
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.
Can't we find a better name?
Because it tests more than just the presence of process
.
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.
Hmm, might be a good idea. Do you have any suggestions? I don't really know what else to call it.
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.
hasGlobalProcessEventListener()
?
It's far from pretty but IMHO it is a bit clearer.
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.
What do you think of hasGlobalProcessEventEmitter()
? Seems a bit more correct
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.
Yes, my bad.
Monday morning and I am already tired :p
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.
Hehe, I feel exactly the same at the moment :D
Done and done 👍 |
Better check for `process.on()`.
Fixes #78
It also includes #76 and #77 so please pull those before pulling this and this pull request should automatically be updated to only show the one relevant commit.