Js typed array methods #3481

AngeloChecked · 2023-11-25T21:06:13Z

This Pull Request pertains to issue #3252

It implements the following JsTypedArray methods:

get buffer
constructor
copyWithin
find_index
find_last
find_last_index
foreach
includes
set
subarray
toLocaleString*
toStringTag

Hi guys!
I'd like to create this draft pull request to share my work on the issue through examples and seek feedback. Is that alright with you? I'll gradually compile this post as I make progress on the issue.

jedel1043

Really nice work! Just have some suggestions.

boa_examples/src/bin/jstypedarray.rs

boa_engine/src/object/builtins/jstypedarray.rs

codecov · 2023-11-28T08:08:13Z

Codecov Report

Attention: Patch coverage is 1.53846% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 47.01%. Comparing base (6ddc2b4) to head (a9cc900).
Report is 43 commits behind head on main.

Files	Patch %	Lines
core/engine/src/object/builtins/jstypedarray.rs	0.00%	59 Missing ⚠️
core/engine/src/builtins/typed_array/builtin.rs	16.66%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3481      +/-   ##
==========================================
- Coverage   47.24%   47.01%   -0.24%     
==========================================
  Files         476      477       +1     
  Lines       46892    47494     +602     
==========================================
+ Hits        22154    22327     +173     
- Misses      24738    25167     +429

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

jedel1043 · 2023-12-03T23:08:14Z

It appears that there's an overlap between TypedArray.prototype.set() and the functionality defined in https://tc39.es/ecma262/#sec-set-o-p-v-throw. I'm uncertain how to resolve this conflict. I've renamed it to typed_array_set, but I'm open to any suggestions or considerations.

Should probably be renamed to set_values, because the original method allows setting multiple values at the same time.

jedel1043 · 2023-12-04T23:52:46Z

While exploring toLocaleString and attempting to replicate the example from MDN, I encountered a localization problem that prevented the code from working as expected. I'm unsure if this issue is widespread. As a temporary solution, I've commented out the example code for now.

Don't worry about that. We haven't implemented the NumberFormat API, meaning our numbers always format to plain numbers. When we implement that, this should be fixed.

AngeloChecked · 2023-12-09T22:47:57Z

jedel1043 · 2023-12-09T23:08:12Z

Can we review each implementation to align with the desired feature and reduce the utilization of JsValue?

Yep, it'll be good to check already implemented functions to see if we can improve the API on them, but that can be done in another PR.

Any suggestions or tips on how to achieve this?

Probably to check the JS implementation to see if there are any patterns on the return types of the methods; methods returning only JsValue::String should return JsString on the object wrapper, methods returning either undefined or a specific type should return Option<Type>, etc.

jedel1043 · 2023-12-09T23:14:21Z

Also, when you feel like this is ready, just ping me so that I can change the label to waiting-on-review

AngeloChecked · 2023-12-22T14:58:26Z

hi @jedel1043

pub fn buffer(&self, context: &mut Context) -> JsResult<JsValue>
pub fn constructor(&self, context: &mut Context) -> JsResult<JsValue>
pub fn copy_within<T>( &self, target: T, start: u64, end: Option<u64>, context: &mut Context,) -> JsResult<Self> where T: Into<JsValue>
pub fn find_index( &self, predicate: JsFunction, this_arg: Option<JsValue>, context: &mut Context,) -> JsResult<Option<u64>> 
pub fn find_last( &self, predicate: JsFunction, this_arg: Option<JsValue>, context: &mut Context,) -> JsResult<JsValue>
pub fn find_last_index( &self, predicate: JsFunction, this_arg: Option<JsValue>, context: &mut Context,) -> JsResult<Option<u64>>
pub fn foreach( &self, callback: JsFunction, this_arg: Option<JsValue>, context: &mut Context,) -> JsResult<JsValue> 
pub fn includes<T>( &self, search_element: T, from_index: Option<u64>, context: &mut Context,) -> JsResult<bool> where T: Into<JsValue>
pub fn set_values( &self, source: JsValue, offset: Option<u64>, context: &mut Context,) -> JsResult<JsValue>
pub fn subarray(&self, begin: i64, end: i64, context: &mut Context) -> JsResult<Self>

from your point of view should any of these fn signs be improved, make the types more explicit?
otherwise, I think I'll go with the review, considering that any side improvements can be put in another card.

AngeloChecked · 2024-01-28T16:34:32Z

hi @jedel1043,

It's been a while. What are our next steps? Did I forget to complete something specific? Can I assist with any tasks for the review?

nekevss

This looks great overall! Really nice job!

I had just a couple nits.

nekevss · 2024-01-29T23:21:13Z

core/engine/src/object/builtins/jstypedarray.rs

+
+    /// Calls `TypedArray.prototype.forEach()`.
+    #[inline]
+    pub fn foreach(


nit: Should this be in snake case?

This is extending the API from BuiltinTypedArray, but we use for_each in other places in the engine. Would it be better to maintain consistency?

Hi @nekevss, big thanks for your support! Apologies for the delay; I got tied up with something else. Now, I'm eager to maintain continuity with this.

I used foreach because the internal API BuiltinTypedArray::foreach uses it too. But I'm flexible to rename it for consistency with the right naming conventions also in the internal api, following your guidance.
If you approve, I'll proceed to rename both. Otherwise, do we prefer to keep the internal API unchanged? give me feedback.

@jedel1043 any thoughts on the use of snake case with foreach on the internal API for BuiltinTypedArray.

In general, I'm of the opinion for at least the external API to be closer to the Rust convention. Anyone who consumes the API will probably expect snake case. At minimum, we should probably be at least consistent throughout the engine on whether it's foreach or for_each.

I think we mostly agree on always converting camelCase JS APIs to snake_case Rust APIs. IMO, functions that differ from this convention should be treated as naming bugs.

Sounds good! 😄 Then let's go with renaming the functions from foreach to for_each for naming.

nekevss · 2024-01-29T23:44:11Z

core/engine/src/object/builtins/jstypedarray.rs

@@ -273,6 +358,104 @@ impl JsTypedArray {
        )
    }

+    /// Calls `TypedArray.prototype.findIndex()`.


nit: can there be some examples added to these docs.

Since JsTypedArray et al. are meant to be an externally facing API. Some more thorough examples in the documentation would be nice. Something a bit more like the examples provided in JsPromise or JsMap.

Do you think it would be more meaningful to enhance the builtins/jstypedarray.rs module with standard Rust documentation, or should we focus solely on improving the code in the examples folder? I drew a lot of inspiration from MDN for those examples, because originally I hadn't thought about it; I used them as acceptance tests to ensure the implementation was correct. However, I'm open to reconsidering them as documentation if that would be more beneficial.

The rule of thumb is to have examples that demonstrate HOW to use the APIs as doc comments, and to have examples that demonstrate WHEN to use the APIs as code examples.

The module level rustdoc documentation was more what I meant in this instance. The examples don't have to be a completely different from those in examples. Mostly since the Rust documentation will be auto generated into the docs, which is super handy and may be the first place someone consuming the API would go for information. For example, here's the JsPromise docs for reference

ok, here i created the first rust doc test

the preview looks like:

I intend to keep drawing considerable inspiration from MDN for comments. What are your thoughts? Should I proceed with implementing Rust doc tests throughout the module? Do you have any specific considerations or tips to share?

Looks really nice! I think it's a good call to adapt the examples from MDN, since it removes the effort of having to come up with descriptive usages.

As a small suggestion, I would remove the "The FindIndex method of Array instances" prefix, since it is mostly idiomatic to start a method's documentation with a verb; in this case, "Returns the index of...".

I've added doc tests to all the functions I've touched. Give me feedback!

nekevss

This is looking great! I have a one nit about the docs, but nothing else really.

core/engine/src/object/builtins/jstypedarray.rs

nekevss

LGTM! Thanks for contributing to Boa! 😄

EDIT: Looks like this needs a cargo fmt -all

AngeloChecked · 2024-02-24T20:13:35Z

LGTM! Thanks for contributing to Boa! 😄

EDIT: Looks like this needs a cargo fmt -all

Done!

And sure... please close this PR. However, if we want to discuss the points further:

i don't know how to implement it

toString

iterators are unstable

@https://github.com/iterator
values
keys
entries

desired features

Easily return the inner data as &[u8] or &Vec

I'm open to contribute. I'm also open to adding(the remaining) Rust test documentation to the jstypedarray module in another pull request. What do you think?

jedel1043 · 2024-02-24T21:20:57Z

Easily return the inner data as &[u8] or &Vec

I think it's fine to only expose this in ArrayBuffer and SharedArrayBuffer. We already allow getting the inner buffer of the typed array, and projecting to another inner object would mean handling two runtime borrows at the same time, which is not ideal.

jedel1043

Really nice work! It's also really helpful to have so many new doc tests for this wrapper. There are definitely some other optimizations that can be done, but having the methods in place is already a big improvement.

This comment was marked as resolved.

Sign in to view

jedel1043 requested changes Nov 26, 2023

View reviewed changes

AngeloChecked force-pushed the JsTypedArray_methods branch 3 times, most recently from bdfaab0 to 66b6a6d Compare November 28, 2023 07:24

jedel1043 self-assigned this Nov 29, 2023

jedel1043 added the waiting-on-author Waiting on PR changes from the author label Nov 29, 2023

This comment was marked as resolved.

Sign in to view

AngeloChecked force-pushed the JsTypedArray_methods branch from 61106e1 to f1c29d2 Compare November 30, 2023 20:51

AngeloChecked force-pushed the JsTypedArray_methods branch from 270dded to 5a28e12 Compare December 4, 2023 22:32

This comment was marked as resolved.

Sign in to view

AngeloChecked force-pushed the JsTypedArray_methods branch from 265202a to c291beb Compare December 9, 2023 22:34

jedel1043 added waiting-on-review Waiting on reviews from the maintainers enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics and removed waiting-on-author Waiting on PR changes from the author labels Dec 25, 2023

AngeloChecked force-pushed the JsTypedArray_methods branch from c291beb to a87e84f Compare January 9, 2024 00:24

AngeloChecked marked this pull request as ready for review January 9, 2024 00:25

nekevss requested changes Jan 29, 2024

View reviewed changes

AngeloChecked force-pushed the JsTypedArray_methods branch 2 times, most recently from 703411b to c5e0b76 Compare February 18, 2024 16:30

nekevss reviewed Feb 22, 2024

View reviewed changes

core/engine/src/object/builtins/jstypedarray.rs Outdated Show resolved Hide resolved

AngeloChecked force-pushed the JsTypedArray_methods branch from f0f816e to 81cdd22 Compare February 23, 2024 23:39

nekevss approved these changes Feb 24, 2024

View reviewed changes

AngeloChecked and others added 16 commits February 24, 2024 21:00

implemented jstypedarray find_index

f1cc2bc

implemented find_last

05740b1

implemented find_last_index

fb7d933

implemented forEach

bce0e4c

implemented includes

2dfce4b

implemented set

dbd7dba

implemented subarray

d01a25c

implemented get buffer

2ce0263

implemented constructor

59a130e

implemented copyWithin

b909980

implemented toLocaleString

e0b8a39

implemented toStringTag

d4b03f1

make linter happy

2dd7935

doc test for buffer and update example

d7285a8

rename foreach to for_each

be8401e

format

a9cc900

AngeloChecked force-pushed the JsTypedArray_methods branch from 81cdd22 to a9cc900 Compare February 24, 2024 20:03

jedel1043 approved these changes Feb 24, 2024

View reviewed changes

jedel1043 added this pull request to the merge queue Feb 24, 2024

Merged via the queue into boa-dev:main with commit 14b34fe Feb 24, 2024
14 checks passed

nekevss added this to the v0.18.0 milestone Feb 28, 2024

nekevss mentioned this pull request Feb 28, 2024

initial changelog for v0.18 #3691

Merged

jedel1043 removed the waiting-on-review Waiting on reviews from the maintainers label Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Js typed array methods #3481

Js typed array methods #3481

AngeloChecked commented Nov 25, 2023 •

edited

Loading

This comment was marked as resolved.

jedel1043 left a comment

codecov bot commented Nov 28, 2023 •

edited

Loading

This comment was marked as resolved.

jedel1043 commented Dec 3, 2023

This comment was marked as resolved.

jedel1043 commented Dec 4, 2023

AngeloChecked commented Dec 9, 2023 •

edited

Loading

jedel1043 commented Dec 9, 2023

jedel1043 commented Dec 9, 2023

AngeloChecked commented Dec 22, 2023

AngeloChecked commented Jan 28, 2024

nekevss left a comment

nekevss Jan 29, 2024

AngeloChecked Feb 14, 2024 •

edited

Loading

nekevss Feb 14, 2024 •

edited

Loading

jedel1043 Feb 14, 2024

nekevss Feb 14, 2024

nekevss Jan 29, 2024

AngeloChecked Feb 14, 2024

jedel1043 Feb 14, 2024 •

edited

Loading

nekevss Feb 14, 2024

AngeloChecked Feb 17, 2024

jedel1043 Feb 17, 2024

AngeloChecked Feb 18, 2024

nekevss left a comment

nekevss left a comment •

edited

Loading

AngeloChecked commented Feb 24, 2024 •

edited

Loading

jedel1043 commented Feb 24, 2024 •

edited

Loading

jedel1043 left a comment

Js typed array methods #3481

Js typed array methods #3481

Conversation

AngeloChecked commented Nov 25, 2023 • edited Loading

This comment was marked as resolved.

jedel1043 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

This comment was marked as resolved.

jedel1043 commented Dec 3, 2023

This comment was marked as resolved.

jedel1043 commented Dec 4, 2023

AngeloChecked commented Dec 9, 2023 • edited Loading

jedel1043 commented Dec 9, 2023

jedel1043 commented Dec 9, 2023

AngeloChecked commented Dec 22, 2023

AngeloChecked commented Jan 28, 2024

nekevss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngeloChecked Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

nekevss Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss left a comment

Choose a reason for hiding this comment

nekevss left a comment • edited Loading

Choose a reason for hiding this comment

AngeloChecked commented Feb 24, 2024 • edited Loading

jedel1043 commented Feb 24, 2024 • edited Loading

jedel1043 left a comment

Choose a reason for hiding this comment

AngeloChecked commented Nov 25, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

AngeloChecked commented Dec 9, 2023 •

edited

Loading

AngeloChecked Feb 14, 2024 •

edited

Loading

nekevss Feb 14, 2024 •

edited

Loading

jedel1043 Feb 14, 2024 •

edited

Loading

nekevss left a comment •

edited

Loading

AngeloChecked commented Feb 24, 2024 •

edited

Loading

jedel1043 commented Feb 24, 2024 •

edited

Loading