Feature native class objects (NativeObject and Class traits)
#627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HalidOdat
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #627 +/- ##
==========================================
- Coverage 73.21% 72.90% -0.32%
==========================================
Files 191 192 +1
Lines 13905 13988 +83
==========================================
+ Hits 10181 10198 +17
- Misses 3724 3790 +66
Continue to review full report at Codecov.
|
Benchmark for cb32a76Click to view benchmark
|
This looks great! But we need something to add methods to the object, constructors and so on. Maybe we could have some |
Yep. right now it's very minimal! It would be nice to have the contexts setup first. |
Merged
Benchmark for 0118483Click to view benchmark
|
About this. This is more of a class than an object, so pub trait Class: NativeObject {
const NAME: &'static str; // This is the binging name of the class
const LENGTH: usize = 0; // Class `length` (the amount of arguments it takes) default is 0
/// The constructor of the class
fn constructor(this: &Value, args: &[Value], ctx: &mut Interpreter) -> ResultValue;
/// Initialize class methods
fn methods(class: &mut ClassBuilder<'a>) -> ResultValue;
}Now with #[derive(Debug, Trace, Finalize)]
struct Person {
name: String,
age: u32,
}
impl Person {
fn say_hello(this: &Value, _: &[Value], ctx: &mut Interpreter) -> ResultValue {
if let Some(object) = this.as_object() {
if let Some(person) = object.downcast_ref::<Person>() {
println!(
"Hello my name is {}, I'm {} years old",
person.name, person.age
);
return Ok(Value::undefined());
}
}
ctx.throw_type_error("'this' is not a Person object")
}
}
impl Class for Person {
const NAME: &'static str = "Person";
const LENGTH: usize = 2;
fn constructor(this: &Value, args: &[Value], ctx: &mut Interpreter) -> ResultValue {
let name = ctx.to_string(&args.get(0).cloned().unwrap_or_default())?;
let age = ctx.to_uint32(&args.get(1).cloned().unwrap_or_default())?;
let person = Person {
name: name.to_string(),
age,
};
this.set_data(ObjectData::NativeObject(Box::new(person)));
Ok(this.clone())
}
fn methods(class: &mut ClassBuilder) -> ResultValue {
// This puts it in `Person.prototype.sayHello`.
class.method("sayHello", 0, Self::say_hello);
// This puts it in `className.iAmAStaticMethod`.
class.static_method("iAmAStaticMethod", 0, |_, _, _| {
println!("Hi o/");
Ok(Value::undefined())
});
Ok(Value::undefined())
}
}
fn main() {
let realm = Realm::create();
let mut context = Interpreter::new(realm);
// Initalizes, and registers the Class.
context.register_global_class::<Person>().unwrap();
forward(
&mut context,
r"
let person_instance = new Person('John', 19);
person_instance.sayHello();
Person.iAmAStaticMethod();
");
}This prints as expected: What do you think? @Razican @jasonwilliams @Lan2u |
HalidOdat
changed the title
NativeObject and Class traits)
I love this! It's very ergonomic and easy to use :) About the API itself, I just have these of questions:
|
I don't think this is going to be an issue either, I can make
Did not read the spec either 😄 , but the constructor is what is called when we do
As far as I know class constructors always require class X {}
x() // Uncaught TypeError: class constructors must be invoked with 'new'I could add a pub trait Class: NativeObject {
const CALLABLE: bool = false; // this would default to false to match with javascript class.
}we could have the same for
I'm not that experienced with rust to C ffi. but this class is a wrapper around what we do in for example |
I think @jasonwilliams can give us some insight here, since he worked with this at some point. |
|
Now in the constructor, instead of this: fn constructor(this: &Value, args: &[Value], ctx: &mut Interpreter) -> ResultValue {
let name = ctx.to_string(&args.get(0).cloned().unwrap_or_default())?;
let age = ctx.to_uint32(&args.get(1).cloned().unwrap_or_default())?;
let person = Person {
name: name.to_string(),
age,
};
this.set_data(ObjectData::NativeObject(Box::new(person)));
Ok(this.clone())
}Instead of having to manually call // _this is still avalable if we want to set properties (string, symbol) of the class instance.
fn constructor(_this: &Value, args: &[Value], ctx: &mut Interpreter) -> Result<Self, Value> {
let name = ctx.to_string(&args.get(0).cloned().unwrap_or_default())?;
let age = ctx.to_uint32(&args.get(1).cloned().unwrap_or_default())?;
let person = Person {
name: name.to_string(),
age,
};
Ok(person)
}what do you think? |
Benchmark for eca45f4Click to view benchmark
|
Benchmark for a4e570fClick to view benchmark
|
HalidOdat
force-pushed
the
feature/native-object
branch
from
fd36121
to
03454bb
Compare
Benchmark for 3ab4b22Click to view benchmark
|
Much better! This also allows us to change the |
HalidOdat
force-pushed
the
feature/native-object
branch
from
03454bb
to
9fd6b53
Compare
Benchmark for 4a9ec1eClick to view benchmark
|
No, constructors and objects are completely separate. A constructor is just a function that’s given an object (this) to then populate and return. You can have objects from scratch with no constructor at all. This is where things get interesting! For ‘new String()’: For ‘String()’: the return value is used and returned to the assignment, all of the For the rest I’ll have to look when I’m back at my desk, but constructable sounds about right, I think this exists in the spec. |
Benchmark for 7d99582Click to view benchmark
|
HalidOdat
force-pushed
the
feature/native-object
branch
from
a057c58
to
d0e8b03
Compare
Benchmark for d6565c7Click to view benchmark
|
Razican
reviewed
Aug 19, 2020
Closed
It is is possible just needs some changes also some missing things are the ability to add static properties and prototype properties, |
In that case, I think we should leave that for a future PR and merge this as soon as you feel it's ready :) |
Benchmark for 272e2e9Click to view benchmark
|
HalidOdat
force-pushed
the
feature/native-object
branch
from
6e8e634
to
e4d3dac
Compare
|
Added the ability to add inherited and static properties as discussed previously. This is ready for review/merge :) |
Benchmark for 57b5c9dClick to view benchmark
|
Razican
reviewed
Aug 21, 2020
Benchmark for 1e1e424Click to view benchmark
|
Razican
approved these changes
Aug 27, 2020
HalidOdat
commented
Aug 27, 2020
- Removed internal state - Added native class example
HalidOdat
force-pushed
the
feature/native-object
branch
from
b2d1790
to
11de16a
Compare
Benchmark for 1985c78Click to view benchmark
|
Benchmark for 49fda3bClick to view benchmark
|
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to #445 and #446
It changes the following:
NativeObjecttraitDebug,Any+Traceit automatically is anNativeObjectTracetrait so this is not unsoundinternal stateClassfor native class creation. see example inboa/examples.