Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move root method to Value trait #996

Open
kjvalencik opened this issue Aug 7, 2023 · 1 comment
Open

Move root method to Value trait #996

kjvalencik opened this issue Aug 7, 2023 · 1 comment
Labels
beginner friendly breaking change rust Pull requests that update Rust code v1.0

Comments

@kjvalencik
Copy link
Member

Currently the .root(cx) method is provided by the object trait Object. This is because Node-API requires the value be an Object.

However, this is an overly restrictive requirement in Node-API and may be relaxed in the future to allow non-Object types (e.g., string).

If we want to support root for non-object types in the future, we will want to move the method. However, moving is a breaking change. Duplicating it is also a breaking change because users may end up with an ambiguous call if both traits are in scope.

I propose moving the function to the Value trait with a T: Object bound. We can relax this bound in the future without a breaking change.

trait Value {
    fn root<'cx, C>(&self, cx: &mut C) -> Root<Self>
    where
        C: Context<'cx>,
        Self: Object + Sized,
    {
        Root::new(cx, self)
    }
}
@kjvalencik kjvalencik added breaking change beginner friendly v1.0 rust Pull requests that update Rust code labels Aug 7, 2023
@dherman
Copy link
Collaborator

dherman commented Aug 11, 2023

I like it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner friendly breaking change rust Pull requests that update Rust code v1.0
Projects
None yet
Development

No branches or pull requests

2 participants