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

IntoJsValue and TryFromJsValue #1995

Closed
wants to merge 14 commits into from

Conversation

lastmjs
Copy link
Contributor

@lastmjs lastmjs commented Mar 31, 2022

I'd love to get feedback on this, especially how I should handle errors and any other idiomatic Rust improvements you'd like me to make (I'm still kind of new to Rust).

This pull request also depends on making the Array builtin public, and in my opinion all of the builtins should be made public. I can open another pull request to make all of those public if I get the go-ahead.

This pull request also depends on implementing my opinion that i128, u128, i64 and u64 should always be converted into a JS bigint since it's the only native JS number type that will not lose precision. More discussion here: #1991

This Pull Request fixes/closes #1975.

It changes the following:

  • Adds IntoJsValue trait with many implementations
  • Add TryFromJsValue trait with many implementations

@lastmjs lastmjs changed the title Into and from js value IntoJsValue and TryFromJsValue Mar 31, 2022
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

I mostly agree with the changes, but I think there are some API questions that need to be discussed.

Comment on lines 106 to 115
impl<T: IntoJsValue> IntoJsValue for Vec<T> {
fn into_js_value(self, context: &mut Context) -> JsValue {
let js_values = self.into_iter().map(|item| item.into_js_value(context)).collect::<Vec<JsValue>>();

Array::create_array_from_list(
js_values,
context
).into()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Exposing Array is pretty unnecessary, since there's already an API that allows to convert any Iterator<Item=JsValue> into an array.

https://github.com/boa-dev/boa/blob/main/boa_engine/src/object/jsarray.rs

Copy link
Contributor Author

@lastmjs lastmjs Mar 31, 2022

Choose a reason for hiding this comment

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

Yeah I'm a bit confused on how to do this the right way, can you give me a little code snippet hint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think I see, from_iter would give me a JsArray and then I could get it to a JsValue from there, let me work on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been addressed

@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #1995 (c2f8824) into main (80e804c) will decrease coverage by 0.11%.
The diff coverage is 4.22%.

@@            Coverage Diff             @@
##             main    #1995      +/-   ##
==========================================
- Coverage   43.50%   43.38%   -0.12%     
==========================================
  Files         220      222       +2     
  Lines       19962    20014      +52     
==========================================
  Hits         8684     8684              
- Misses      11278    11330      +52     
Impacted Files Coverage Δ
boa_engine/src/bigint.rs 37.22% <0.00%> (-1.12%) ⬇️
boa_engine/src/value/into_js_value.rs 0.00% <0.00%> (ø)
boa_engine/src/value/mod.rs 51.40% <0.00%> (-0.74%) ⬇️
boa_engine/src/value/try_from_js_value.rs 0.00% <0.00%> (ø)
boa_engine/src/value/conversions.rs 18.51% <12.50%> (-1.49%) ⬇️
boa_engine/src/syntax/ast/node/spread/mod.rs 57.14% <0.00%> (-14.29%) ⬇️
...ntax/parser/expression/left_hand_side/arguments.rs 35.00% <0.00%> (-2.50%) ⬇️
boa_engine/src/builtins/regexp/mod.rs 67.61% <0.00%> (-0.18%) ⬇️
...a_engine/src/syntax/parser/statement/switch/mod.rs 39.53% <0.00%> (+1.16%) ⬆️
...a_engine/src/syntax/ast/node/statement_list/mod.rs 78.12% <0.00%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80e804c...c2f8824. Read the comment docs.

@jedel1043
Copy link
Member

Closing in favour of #2276.

@jedel1043 jedel1043 closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TryFromJsValue
3 participants