Discussions on ReScript exception encoding

[BlueHotDog]

@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.

[ryyppy]

@jacobp100

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.

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?

[austindd]

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.

[jfrolich]

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.

[johnj]

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?

[Hongbo]

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

[BlueHotDog]

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.

[jfrolich]

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.

[BlueHotDog]

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…

[Hongbo]

Hi, we share the pain, we may consider doing some incremental improvement soon

[mbergal]

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.

[yawaramin]

How about something like this?

// Helper to throw JavaScript Error
@new external error: string => exn = "Error"
let throw = exn => exn->Printexc.to_string->error->raise

// Suppose this is your app entrypoint
let main = () => assert(false)

// Top-level exn-to-Error converter
let () = try main() catch {
  | exn => throw(exn)
}

[bloodyowl]

@jacobp100

Object.assign(new Error("ReScript Error at tscanf_test.ml (174:2)"), {
    RE_EXN_ID: "Assert_failure",
    _1: [
          "tscanf_test.ml",
          174,
          2
        ]
})

This look like the most practical in terms of DX, if we (for instance) look at the output in Safari dev tools.

[BlueHotDog]

So, this topic is dead. yet we’re now facing the same issue with yet another industry standard tool - DataDog.

Specifically we get the following message from them:
" ERROR STACK

No stack, consider using an instance of Error

"

@Hongbo - or anyone from the ReScript team - how is this not a priority for allowing us running Safely in production?

[Hongbo]

I think it is an important issue to be addressed. We are a small team, and none of us work on it full time, so I guess you need be a little patient

[illusionalsagacity]

Have you considered wrapping the window.onerror property after your analytics error collectors are installed?

// install rescript error shim first
window.addEventListener("error", function rescriptPriorityHandler(event) {
  if (Object.prototype.hasOwnProperty.call(event.error, "RE_EXN_ID")) {
    event.preventDefault();
    event.stopPropagation();
    event.error.Error.message = event.error.RE_EXN_ID;
    throw event.error.Error;
  }
});

// analytics error handlers
const originalOnError = (window.onerror = function analyticsonerror(
  message,
  source,
  lineno,
  colno,
  error
) {
  console.log("Original error handler called!");
});

window.onerror = function rescriptErrorWrapper(
  message,
  source,
  lineno,
  colno,
  error
) {
  if (Object.prototype.hasOwnProperty.call(error, "RE_EXN_ID")) {
    return true;
  } else if (typeof originalOnError === "function") {
    originalOnError.apply(this, arguments);
  }
};

[Hongbo]

The challenges is that a niche language feature-- extensible variants shared the same encoding with exceptions, so it is hard to change the encoding of exception without touching the extensible variants, unless we remove that niche feature…

[tsnobip]

We use some tools (like Graphile-Worker) that are based on catching exceptions that must be JS Errors, having Rescript exceptions encoded as JS Errors would make integration much easier.

We could make a poll, but I would personally easily trend easier JS interop with dropping support for extensible variants, I suspect very few people actually use them.

[tsnobip]

My bad actually, Graphile-Worker doesn’t expect errors to be instance of JS Error, it just expects message and stack fields, so for this tool your proposal #1 would be enough given we can add an inline record with a message field inside the rescript exception and that it would add the stack.

[tsnobip]

Another improvement would be to encode the first element of the payload when it’s a string as message instead of _1, that would allow for a much better JS integration for default exceptions, like Failure(string) (failwith), Invalid_args, etc and for exceptions that have only one string argument and where you don’t want to add the overhead of a adding an inline record with a message field.