boa-dev / temporal

A Rust implementation of ECMAScript's Temporal API
Apache License 2.0
26 stars 4 forks source link

Migrate errors to static enums #113

Open jedel1043 opened 2 days ago

jedel1043 commented 2 days ago

Our current design for TemporalError is essentially a mirror of ECMAScript's error types: https://github.com/boa-dev/temporal/blob/345ad548db498a5fe596ebebfc7f0ea6214fb079/src/error.rs#L39-L43

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs, and a more rusty approach will be better suited for this. thiserror sounds ideal for this (moreso because it recently added support for no_std).

jasonwilliams commented 2 days ago

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Also, what advantage does thiserror bring over using static enums?

jedel1043 commented 1 day ago

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Yep, because the underlying ixdtf uses this design for errors :)

https://docs.rs/ixdtf/latest/ixdtf/enum.ParserError.html

Also, what advantage does thiserror bring over using static enums?

It removes the boilerplate of having to wrap errors generated by inner calls, implement the Error trait and implement the Display trait. Though, we could do it manually if we see that it's not that hard to maintain.

nekevss commented 17 hours ago

Yeah, the initial design was primarily focused on compatibility with Boa's error type. But it probably is not the most optimal error type for a general purpose library.