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

Add JsNativeObject wrapper #3479

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnyob
Copy link
Contributor

@johnyob johnyob commented Nov 24, 2023

This PR adds a JsNativeObject wrapper around JsObject with the invariant that the underlying object data is a native object of some type T.

@johnyob johnyob force-pushed the ajob410@js-native-object branch from ae8fb11 to 5e459a5 Compare November 24, 2023 11:14
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (07e2356) 44.59% compared to head (5e459a5) 44.55%.
Report is 50 commits behind head on main.

Files Patch % Lines
boa_engine/src/object/builtins/jsnative_object.rs 0.00% 40 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3479      +/-   ##
==========================================
- Coverage   44.59%   44.55%   -0.04%     
==========================================
  Files         487      488       +1     
  Lines       50601    50641      +40     
==========================================
  Hits        22563    22563              
- Misses      28038    28078      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnyob
Copy link
Contributor Author

johnyob commented Nov 24, 2023

Are doc tests not covered by codecov?

@jedel1043
Copy link
Member

Are doc tests not covered by codecov?

Nope, and I'm not sure if tarpaulin supports testing docs.

@jedel1043
Copy link
Member

@jedel1043, @raskad and @HalidOdat will try to review this.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition! Thanks for the contribution @johnyob :)

Comment on lines +178 to +194
pub fn new(native_object: T, context: &mut Context) -> JsResult<Self>
where
T: Class,
{
let class = context.get_global_class::<T>().ok_or_else(|| {
JsNativeError::typ().with_message(format!(
"could not find native class `{}` in the map of registered classes",
T::NAME
))
})?;

Ok(Self::new_with_proto(
class.prototype(),
native_object,
context,
))
}
Copy link
Member

@jedel1043 jedel1043 Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that #3488 is merged, this functionality should be replaced by returning a JsNativeObject on Class itself. That should also ensure that any new additions to Class are correctly called (this implementation skips calling the Class::object_constructor method).

@jedel1043 jedel1043 added the waiting-on-author Waiting on PR changes from the author label Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting on PR changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants