It feels more of luck proposal 1 works with some tools - it’s relying upon the implementation details, and any tool that did more probing on the error might fall over. Where as 2 is the established way in JS of throwing errors, so if it’s not supported in a tool, it’s squarely the tool’s fault.
But at the same time, maybe there’s a lot of sense in going proposal 1 if it is a much easier change - and then see if and why it fails in some tools. Then if later down the line it turns out there are issues, there’ll be more information.
If this change is not aiming for seamless interop, then I’d think Proposal 2 is too risky, and proposal 1 is more graceful. Extending instances of builtins is generally not a good idea, so I’d rather implement some catch handler in e.g. mocha and rethrow the unwrapped value afterwards.
i strongly disagree. mocha is one thing. but my production system not able to catch exceptions is a different(and real-life) problem.
Can someone explain why option 3, e.g using the JS way of creating custom exceptions is a no-go?
They may also do err instanceof Error and rely on it being actually an instance of the Error class. IMHO these are all valid ways to check that something is a JavaScript error.
Not to derail too much, did you mean extending builtins or instances of builtins? Extending instances of builtins is really common in JS - especially (or mainly?) for errors and functions. Node has a lot of error creation stuff for attaching things like error codes that works pretty much the same as prop. 2.
I’m a bit surprised to see so much debate about the internal implementation of rescript exceptions, do people here use exceptions so much in rescript? I had the impression the de facto standard was to use the result type, but maybe using exceptions is more common than I thought, what’s your use case for it?
@tsnobip We use them for error logging in our serverless functions. It’s a convenient way to get a stack trace along with the ReScript code location without jumping through too many hoops… But we generally use the result type for error-handling logic. It seems to work well for us.
I agree with @jacobp100 that extending intances of builtins is not as bad as extending prototypes and constructors, and I also like @BlueHotDog’s proposal of using a custom error class. A custom constructor would make it 100% legit, IMHO. Probably more legit than trying to conform to some random implementations (like Mocha, in this case) in an ad hoc way.
Then again, when you frame your question like plain “would you like a feature X?”, users will always say yes, because it seems free for them. If the question was: “do you want Proposal 1 published in a week or Proposal 2 in ~2 month (postponing features Y ans Z)”, or: “Would you rather have Proposal 2 or Feature AA”, maybe we’d answer differently
@tsnobip we rarely use them… but when they happen… i want them to Just Work and not deal with global capture and mutate to try and make some tool happy, or understanding ReScript representations. I’m very confused by this debate. if rescript can be a nice JS Citizen, than it should, that should be IMO a guiding factor in deciding such features.
If this is an internal representation i just honestly dont understand how making a solution that’s not 100% aligned with the standard is a good idea. i never want to know about the internal representation of ReScript. and i think that’s a very healthy thing to keep in mind.
JS is a crazy world… besides Mocha you’ve an endless list of things relaying on exceptions. they should just work.
Yes, I mean extending both, prototype and instances as well. Just because Node does it, doesn’t mean every single web browser and framework respect that, and you will always have some tool not behaving correctly, no matter how you implement your duck-typed error objects.
I mean I do mostly speak from my pre ES6 experience, because I ran into a lot of footguns with extending objects arbitrarily with my own values, and I am sure I am not alone.
Regarding ES6 class inheritance: Isn’t this feature extremely dependent on the browser / node version? Just looking at the MDN doesn’t give me any good feeling for easy integration in old babel codebases or older browser targets.
EDIT: Also, I am probably fine with whatever is being implemented, I am just throwing out my thoughts, because apparently this is what Bob wanted when opening this topic?
My thoughts: I prefer something more like proposal 2, but I can see how the internal metadata could get removed (or maybe even modified) by third party tools.
One example is error normalization, where a library might assume it’s a standard Error instance and copy it with a modified message or stack property. If they aren’t copying all the object keys, then this would dump the ReScript metadata.
Granted, this sort of technique is generally frowned upon in the JS world, but I’ve seen it done in some frameworks & popular libraries.
It looks like option two is the least problematic. I would just like to mention that I would choose a more unique property than _1 for the metadata, maybe RE_EXN_1.
Btw we can also set the message/name etc of the error to sane values (like the same as RE_EXN_ID.
Btw2: A custom error class is probably the best way to go, compliant and in line with how this would be solved in JavaScript.
Since extensible variants and exceptions share the same implementation, how much does that factor into the decision? For example, if exceptions were compiled to a custom error class, would that make all extensible variants that same class? Or, if they did, would that be a problem?
That’s one of underlying complexities and explains why I tend to proposal 1.
I understand the advantages of proposal 2 vs proposal 1, so repeating this is not that helpful.
What I asked is if there are any practical disadvantages (e.g, break some popular 3rd party frameworks) if we go to proposal 1, at best with some concrete examples
I don’t understand this question… how am i supposed to know if the new implementation will work with the tooling i use(or their internal tooling) without trying it in prod?
I just want to stress how frustrating it was to debug this issue previously and how i wish to not ever do this again:
Since it broke the standard Bugsnag was relying on, it took me 2 days of real bugs in production!! to figure out ReScript was doing something weird(i was suspecting Bugsnag at first, because who will encode an exception differently, i thought to myself).i would imagine anyone less committed to rescript to just drop it and move on.
If one of the premises of ReScript is to produce a SANE Js -> proposal 1 is a no-go. - lets be true to our marketing.
Yeah. I agree. Exceptions is a surface area with the outside world (JS). So it would be nice to have proper JS-conform exceptions. I think solution 1 is a small step in the right direction, but something like solution 2 which might be harder to implement in a clean way is probably something that should be on the longer term roadmap to make it fit in better with the JS ecosystem. If the implementation of 1 does not help us to eventually get to 2, I think effort is probably spent better on 2 in the longer term.
Any update regarding this?
I would say this is probably the most painful point for us using rescript in the real world(to the point that i’ve not idea how other people are doing this… - happy to learn)
We’re using Sentry to capture FE exceptions(used to use bugsnag where it also didnt work).
All our exception look like:
Error: Non-Error exception captured with keys: Error, RE_EXN_ID
Which, compounded with the fact that rescript doesnt have sourcemaps, and that webpack/react makes the trace impossible to read - makes us ignore production exceptions, which is just terrible for a language that one of the selling point is Stability on production…
In the server apps, we translate on the Rescript/JavaScript boundary.
Also, errors in Rescript are not signalled by exceptions only, they can be returned as result<'a,'e>. So just a better encoding might not solve the whole problem for us.
