Use `toString` check rather than `instanceof` check. by joneshf · Pull Request #3 · purescript/purescript-exceptions

joneshf · 2014-09-02T18:16:06Z

Re: discussion in #1 Should fix wonky js context things.

This seems to provide the semantics of what the type specifies, since throwing if we didn't immediately get an Error actually introduces back an Exception effect. which catchException is supposed to remove.

paf31 · 2014-09-02T18:21:24Z

Seems like the issue with this is that I can define my own Error type and throw it, and it could be just as type-unsafe. My vote would be to do what you suggested and wrap everything that isn't instanceof Error in a constructor and pass it to the continuation.

joneshf · 2014-09-02T19:03:30Z

Well, if it's a user defined type, the toString call will give back '[object Object]' so it would fail the first check and be wrapped into a Error and handled.

That sort of raises the question, what do you do for user defined Error. Maybe we should have more than one check in here:

instanceof - to handle built-in Error hierarchy and user defined in the same context
toString - to handle built-in Error hierarchy in a different context (iframe, NodeWebKit, etc)
check for the only standardized properties name and message - to handle user defined in different context

In any other case, just wrap in an Error and handle it.

joneshf · 2014-09-02T19:05:01Z

Also, since I'm not fully confident in js's prototypal semantics, It'd be great to hear some feedback from others like @michaelficarra

paf31 · 2014-09-02T19:05:24Z

I'd like to keep this as simple as possible. Why not just if something is an instanceof Error, we pass it to the continuation unchanged, and if not, we wrap its .toString() in an Error and pass that?

michaelficarra · 2014-09-02T19:34:45Z

{}.toString.call(e) === '[object Error]'

is more idiomatic.

I guess that wouldn't match TypeError and the like though?

Oh, nevermind, nor would the slice.

joneshf · 2014-09-02T19:57:07Z

I agree, the simpler the better. Unfortunately, I think that the ugliness of js will either show up in this library or with users of the library.

If we stringify and wrap things that fail the instanceof check in an Error and you're handling an exception from a different context (iframe or something like node's vm), you end up with a few issues: the name of the error is lost, you have to parse the string to get the actual message, and, as you mentioned, the stack is not correct (even if it's non-standard). There might be more here, but those are the ones that come to mind.

The name bit might be less important if we provide finer control over Error subtypes
The message part means you have to parse out the Error: in order to get to it, not that big a deal, but something to be awareof.
The stack part is non-standard so it really shouldn't be a concern, but I think most common engines implement it anyway.

I think we actually lose a bit of expressivity by just wrapping anything that doesn't pass the instanceof check, since we wouldn't be able to properly handle an exception from a different context as we would lose some information in the toString call.

I might be overthinking it. Or, more likely, underthinking it. Or, maybe it's more unsafe to have these separate checks. But it feels like someone is going to get the ugliness of this, and mitigating it to the library sounds like the better way.

joneshf · 2014-09-02T19:58:49Z

Actually, I guess since the toString would prefix the name of the Error to the message the first two points don't really matter much.

Hmm, I think I'm actually okay with this.

joneshf · 2014-09-02T20:08:30Z

Alright, let me know of any other changes.

michaelficarra · 2014-09-03T16:58:03Z

Why did you remove the cross-realm support? I think that is useful for people working in a browser environment.

I agree entirely that this is very useful for these different scenarios. I think to be robust, we should handle more cases (at least the three mentioned here). But, it also seems like you're not losing much information if you parse the message returned. You lose the stack, but that's non-standard anyway. Maybe it's more important to support that as well?

My thoughts on this whole situation is that I'd rather not have to deal with js semantics at all and especially not exceptions. At this point, if something is thrown somewhere, it's more than likely something that could have been caught on the ps side if it were implemented there. So, until more things are implemented there, I'm fine with dealing with partially implemented solutions. I don't actually care enough what catchException does so long as I can handle it. It might not be pretty or robust, but so long as it's not a runtime error I can deal with it. And, if partially solving this at the moment is what it takes to get this merged, I'd rather that. I'm surely not the only one who has (or will) run into this issue, and I'd rather there be some fix than none at all.

Not mad, not angry, not trying to be condescending or snarky or anything like that, just wanting to explain my thoughts over the internet :).

Oh, and if you have a different or better way to do this, please, relay the info or submit a pr or whatever.

As I said, I don't really care what happens here, so long as it's not a runtime error I can't do anything about.

I think it's worth the slightly less clear (but common and idiomatic) {}.toString call. If there was a downside to doing it the right way, I'd be 100% on your side, but it doesn't hurt anything in this case.

But then, don't we miss out on a user/library defined Error type? Since {}.toString.call(myError) === '[object Object]'. Or is there a way to make this work as well?

Again, I'm indifferent either way, because at least we've not got a runtime error now. But, I don't think this can be a simple one case check. There's multiple ways to construct an error that should be caught here, and we should have cases for each if we're going to do it right.

@paf31 thoughts?

I really only care to handle the Error type, and to wrap other things. If we can handle Errors from other domains, that's a plus in my book.

I'll let you both decide :)

Alright, so @michaelficarra what say you?

Just the one {}.toString.call(e).slice(8, -1) === 'Error' check, or also an instance check so we catch user/lib defined Error's as well?

joneshf · 2014-09-03T21:25:00Z

Alright, can we call this good? or more changes?

paf31 · 2014-09-03T21:25:56Z

Yeah, looks good to me.

michaelficarra · 2014-09-04T16:05:59Z

Just the one {}.toString.call(e) ... check, or also an instance check so we catch user/lib defined Error's as well?

👍, do both. That should achieve cross-realm support as well as Error subclass support.

Use `toString` check AND `instanceof` check.

joneshf · 2014-09-04T16:58:11Z

Okay. Good all around? @paf31 @michaelficarra ?

paf31 · 2014-09-04T16:59:22Z

🏁

michaelficarra · 2014-09-04T17:20:00Z

💝 Perfect.

joneshf · 2014-09-04T22:37:26Z

How about over here 😉

Use `toString` check rather than `instanceof` check.

michaelficarra reviewed Sep 2, 2014
View reviewed changes

joneshf force-pushed the master branch from a0a7b43 to caa4a05 Compare September 2, 2014 20:07

michaelficarra reviewed Sep 3, 2014
View reviewed changes

joneshf force-pushed the master branch from caa4a05 to e8a840f Compare September 3, 2014 21:23

Wrap caught "Error" in an Error.

5934815

Use `toString` check AND `instanceof` check.

joneshf force-pushed the master branch from e8a840f to 5934815 Compare September 4, 2014 16:57

paf31 added a commit that referenced this pull request Sep 4, 2014

Merge pull request #3 from joneshf/master

4db7688

Use `toString` check rather than `instanceof` check.

paf31 merged commit 4db7688 into purescript:master Sep 4, 2014

Conversation

joneshf commented Sep 2, 2014

Uh oh!

paf31 commented Sep 2, 2014

Uh oh!

joneshf commented Sep 2, 2014

Uh oh!

joneshf commented Sep 2, 2014

Uh oh!

paf31 commented Sep 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joneshf commented Sep 2, 2014

Uh oh!

joneshf commented Sep 2, 2014

Uh oh!

joneshf commented Sep 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joneshf commented Sep 3, 2014

Uh oh!

paf31 commented Sep 3, 2014

Uh oh!

michaelficarra commented Sep 4, 2014

Uh oh!

joneshf commented Sep 4, 2014

Uh oh!

paf31 commented Sep 4, 2014

Uh oh!

michaelficarra commented Sep 4, 2014

Uh oh!

joneshf commented Sep 4, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants