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

mimic node more closely #77

Merged
merged 1 commit into from
Jun 17, 2015
Merged

mimic node more closely #77

merged 1 commit into from
Jun 17, 2015

Conversation

LinusU
Copy link
Collaborator

@LinusU LinusU commented Jun 16, 2015

This is the fix first suggested by @crispy1989 in #46 but tweaked to work better and have correct tests.

Node's default behavior is to only print an exception and exit if there is no other uncaughtException handler registered. This emulates that default behavior by bypassing the uncaughtException handler if other handlers are registered.

It also includes #76 so pull that before pulling this and this pull request should automatically be updated to only show the one relevant commit.

@evanw @julien-f If you want another maintainer I would be willing to help :)

@LinusU LinusU mentioned this pull request Jun 16, 2015
@LinusU
Copy link
Collaborator Author

LinusU commented Jun 16, 2015

Test passes in browser! Good to merge 👍

@julien-f
Copy link
Collaborator

Please rebase on master to avoid multiple support charset... commits.

@LinusU
Copy link
Collaborator Author

LinusU commented Jun 17, 2015

Done :)

@julien-f
Copy link
Collaborator

I find your implementation a bit complicated, why did you not simply change handleUncaughtException() to check for a single listener (itself)?

@LinusU
Copy link
Collaborator Author

LinusU commented Jun 17, 2015

You could read some of the reasoning from when I did a similar thing before here: segment-boneyard/oh-crap#1

tl;dr If another module would to the same thing as us, it would break both.

@julien-f
Copy link
Collaborator

I am not a fan of redefining process.emit() but I see your point.

@julien-f
Copy link
Collaborator

The only thing that worries me is that we have no guarantee that Node will use process.emit() to trigger event, it may use an internal version or even the original definition of emit().

@LinusU
Copy link
Collaborator Author

LinusU commented Jun 17, 2015

Hmm, yeah I see your point. Well, I've read the source code and I know that it works for now. I don't see it as such a big risk thought, if that happens we can always fix it then.

julien-f added a commit that referenced this pull request Jun 17, 2015
Do not handle `uncaughtException` if there are other listeners
@julien-f julien-f merged commit 7c21f3f into evanw:master Jun 17, 2015
@julien-f
Copy link
Collaborator

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants