Enum support for TryIntoJs, structured errors from Result

[mkawalec]

As the title says, this adds support for auto-conversion of enums into JS. Representation used is the same as the default one in Serde. I took the same choices when it comes to unit variants' representation: they are represented by strings with the value being the variant name in PascalCase. Unit structs are also supported, and represented by nulls (see examples/json for a hands on outline of different representation possibilities).

There is one somewhat breaking change buried in here. The representation of TryIntoJs for Result<T, E> no longer demands E: ToString, but requires E: TryIntoJs. This allows the user to read structured errors in JS, no longer having to rely on just their string representation. If you see a way of achieving the same thing without introducing a breaking change, let me know in the review.

[nicholastmosher]

Hello again @mkawalec, thanks for the PR! I'll review it soon and get back with any comments!

[mkawalec]

I'll have to add TryIntoJs for NjError

[nicholastmosher]

Are you able to do this? I'm running into this error when building the examples

error[E0599]: the method `try_to_js` exists for enum `Result<i32, NjError>`, but its trait bounds were not satisfied
   --> function/src/lib.rs:15:1
    |
15  | #[node_bindgen]
    | ^^^^^^^^^^^^^^^ method cannot be called on `Result<i32, NjError>` due to unsatisfied trait bounds
    |
   ::: /Users/nick/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:241:1
    |
241 | pub enum Result<T, E> {
    | --------------------- doesn't satisfy `Result<i32, NjError>: TryIntoJs`
    |
   ::: /Users/nick/infinyon/node-bindgen/nj-core/src/error.rs:12:1
    |
12  | pub enum NjError {
    | ---------------- doesn't satisfy `NjError: TryIntoJs`
    |
    = note: the following trait bounds were not satisfied:
            `NjError: TryIntoJs`
            which is required by `Result<i32, NjError>: TryIntoJs`
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `nj-example-function`

[mkawalec]

Yeah, not yet, I'll update the PR tomorrow.

[sehz]

rebase from master

[sehz]

4.4.0 wasn't release, so we can merged into 4.5 release!

[nicholastmosher]

The wording here is a bit confusing at first glance. How about

Tuple variants will be converted into lists, struct variants converted to objects, and unit variants converted into strings matching the variant's name in PascalCase.

[sehz]

rebase please

[sehz]

To avoid breaking changes, it is possible add auto impl for TryIntoJs for string?

[mkawalec]

@sehz so string-containing errors are converted to strings, but other kinds of errors keep their structure? I think there's a possible solution out there, I'll find one that isn't breaking.

[mkawalec]

To avoid breaking changes, it is possible add auto impl for TryIntoJs for string?

@sehz Unfortunately not, impl<T: ToString> TryIntoJs for T is too wide and encompasses virtually all the types, including the ones which have TryIntoJs defined already. Would you be open to having this breaking change here?

I'll keep on poking on this today, asking in case I don't find a non-breaking solution.

[sehz]

@nicholastmosher idea on how to keep compatibility? If not we will do major semversion. That willl also give up opportunity to clean up other API as well

[nicholastmosher]

Looking now, need to catch up

[nicholastmosher]

Hmm I don't see a way we can do this, because for any given type T, T may implement both ToString and may also implement TryIntoJs for itself, so there would not be a single choice of implementation to use and the compiler would reject it.

I did take a stab at a conversion anyways but it did not work. This is the code I was experimenting with that failed:

enum ErrorConversion<E> {
    StringError(String),
    NativeError(E),
}

impl<S: ToString> From<S> for ErrorConversion<()> {
    fn from(s: S) -> Self {
        Self::StringError(s.to_string())
    }
}

impl<E: TryIntoJs> From<E> for ErrorConversion<E> {
    fn from(e: E) -> Self {
        Self::NativeError(e)
    }
}

impl<T, E, C> TryIntoJs for Result<T, C>
where
    T: TryIntoJs,
    E: TryIntoJs,
    C: Into<ErrorConversion<E>>,
{
    fn try_to_js(self, js_env: &JsEnv) -> Result<napi_value, NjError> {
        match self {
            Ok(val) => val.try_to_js(&js_env),
            Err(err) => {
                let conv = err.into();
                match conv {
                    ErrorConversion::StringError(string) => Err(NjError::Other(string)),
                    ErrorConversion::NativeError(e) => Err(NjError::Native(e.try_to_js(&js_env))),
                }
            }
        }
    }
}

This fails to compile with the following error:

error[E0207]: the type parameter `E` is not constrained by the impl trait, self type, or predicates
  --> nj-core/src/convert.rs:93:9
   |
93 | impl<T, E, C> TryIntoJs for Result<T, C<E>>
   |         ^ unconstrained type parameter

error: aborting due to 2 previous errors

I think we should probably accept that this will be a breaking change and move forward with the major version bump. I think the new enum conversion for errors is better than what we had previously.

[sehz]

K. Let's bump up major version

[sehz]

Since we are bump up major version, let's see what else can be clean up in terms of API.
We should create issue for it

[mkawalec]

Ok, I'll bump the version here and create the issue shortly...

[mkawalec]

Created a stub issue at #156, please comment with any improvements you have in mind.

[mkawalec]

I've noticed there is no example/test which tests the new structured errors from Err, I'll add one before merging.

[sehz]

Great job!