Type extractors for common container types #1091
Conversation
- impl TryIntoJs for RefCell - impl TryFromJs for &RefCell
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
==========================================
+ Coverage 83.05% 83.42% +0.36%
==========================================
Files 73 74 +1
Lines 5472 5538 +66
Branches 5472 5538 +66
==========================================
+ Hits 4545 4620 +75
+ Misses 810 803 -7
+ Partials 117 115 -2 ☔ View full report in Codecov by Sentry. |
- impl TryIntoJs for Rc - impl TryFromJs for Rc - impl TryIntoJs for Arc - impl TryFromJs for Arc - impl TryIntoJs for Mutex - impl TryFromJs for &Mutex - impl TryFromJs for MutexGuard also a couple more RefCell conveniences: - impl TryFromJs for Ref - impl TryFromJs for RefMut
| } | ||
|
|
||
| impl<'cx, T: 'static> TryFromJs<'cx> for &'cx RefCell<T> { | ||
| type Error = RustTypeExpected<RefCell<T>>; |
There was a problem hiding this comment.
What do you think of using TypeExpected<JsBox<T>> to reduce the API surface area a bit? Since these types are still JsBox under the hood, I think it's okay to expose the error as that.
There was a problem hiding this comment.
That may be a good idea also so people can understand what the actual runtime representation is. (I think it would be TypeExpected<JsBox<RefCell<T>>>, right?)
There was a problem hiding this comment.
@kjvalencik A related design call I'd love your input on: right now the dynamic borrow errors are explicitly handled with try_borrow and try_borrow_mut and turned into a Neon-specific error type called RefCellError. An alternative is we could just let them panic and keep the API surface to just TypeExpected. Do you have a preference?
There was a problem hiding this comment.
I also dislike the fact that RefCellError::WrongType overlaps with TypeExpected.
There was a problem hiding this comment.
Yeah I really don't see much tangible benefit to the extra types. The panic message is basically the same. I'll remove it for now and if you disagree we can talk about putting the type back.
There was a problem hiding this comment.
Yes, TypeExpected<JsBox<RefCell<T>>> would be the type I would expect.
For Ref and RefMut, I don't like letting it panic. We'll catch the unwind and convert it to an exception, but it might not be a clear error message (also, the overhead of the unwind/catch).
I think it's okay to let throw an exception. If this behavior is unwanted, the author can go to &'cx RefCell as an intermediary and then borrow. If they are going directly to Ref or RefMut, they are already opting into some shorthand syntax.
However, I think it's better for the try_from_js to call the try_ methods on RefCell and on error, manually throw with cx.throw_error(..) and a clear error message.
There was a problem hiding this comment.
Oh I think I see what you're suggesting: instead of creating a new Error type, just explicitly throw an error with a custom message directly in the TryFromJs implementation. That's reasonable. I'm not sure it makes a huge difference but I'm OK with it.
There was a problem hiding this comment.
OK I pushed this middle ground -- it uses try_borrow() and try_borrow_mut() but explicitly calls cx.throw_error() instead of creating a custom type, so it can use TypeExpected for the other case. This feels like a good choice to me but see what you think.
- just use TypeExpected for type errors - just use panic for dynamic borrow errors - eliminate RustTypeExpected and RefCellError types completely - eliminate Container trait
…comments, but just throw instead of a custom error type
| let v = Box::new(value) as BoxAny; | ||
| // Since this value was just constructed, we know it is `T` | ||
| let raw_data = &*v as *const dyn Any as *const T; | ||
| let local = unsafe { external::create(cx.env().to_raw(), v, finalizer) }; |
There was a problem hiding this comment.
Could we refactor most of this into a shared function called by each of the constructors? It would be nice to avoid duplicating the unsafe.
| impl<T: 'static> JsBox<T> { | ||
| pub(crate) fn manually_finalize<'a, C>(cx: &mut C, value: T) -> Handle<'a, JsBox<T>> | ||
| where | ||
| C: Context<'a>, |
There was a problem hiding this comment.
FYI, since Cx was added, we don't need to be generic over Context, unless we are trying for consistency with an existing public API.
| v: Handle<'cx, JsValue>, | ||
| ) -> NeonResult<Result<Self, Self::Error>> { | ||
| match v.downcast::<JsBox<Rc<T>>, _>(cx) { | ||
| Ok(v) => Ok(Ok(JsBox::deref(&v).clone())), |
There was a problem hiding this comment.
Nit, this could use deref coercion. I might write it as:
Rc::clone(&v)|
|
||
| impl<'a, T> Sealed for Ref<'a, T> {} | ||
|
|
||
| impl<'a, T> Sealed for RefMut<'a, T> {} |
There was a problem hiding this comment.
Clippy: These two impls have unnecessary lifetimes.
Implement type extractors for common container types in the standard library, so that they don't required explicit
Boxedwrapping withJsBoxin userland.Implemented so far:
RefCellimpl TryIntoJs for RefCellimpl TryFromJs for &RefCellimpl TryFromJs for Refimpl TryFromJs for RefMutRcimpl TryIntoJs for Rcimpl TryFromJs for RcArcimpl TryIntoJs for Arcimpl TryFromJs for Arc