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

Tracking issue for JsObject safe wrappers #2098

Open
12 of 16 tasks
jedel1043 opened this issue Jun 1, 2022 · 16 comments
Open
12 of 16 tasks

Tracking issue for JsObject safe wrappers #2098

jedel1043 opened this issue Jun 1, 2022 · 16 comments
Labels
API builtins PRs and Issues related to builtins/intrinsics E-Medium Medium difficulty problem enhancement New feature or request help wanted Extra attention is needed

Comments

@jedel1043
Copy link
Member

jedel1043 commented Jun 1, 2022

This is the main tracking issue listing all the JsObjects for which we could implement safe wrappers, and the implementation progress for them.

Feel free to leave a comment if there's something missing.

@jedel1043 jedel1043 added enhancement New feature or request help wanted Extra attention is needed builtins PRs and Issues related to builtins/intrinsics API E-Medium Medium difficulty problem labels Jun 1, 2022
@nekevss
Copy link
Member

nekevss commented Jun 5, 2022

Hi! Would it be fine to take a crack at one or two of these or is someone currently working on them?

@Razican
Copy link
Member

Razican commented Jun 5, 2022

Hi! Would it be fine to take a crack at one or two of these or is someone currently working on them?

Go ahead! Let us know in which ones you'll start working on :)

@nekevss
Copy link
Member

nekevss commented Jun 9, 2022

@Razican I'll start working on JsMap if no one else has started on it

@jedel1043
Copy link
Member Author

@Razican I'll start working on JsMap if no one else has started on it

Nice! I'll edit the description to assign you that work :)

@lameferret
Copy link
Contributor

@jedel1043 can I give JsSet a shot?

@jedel1043
Copy link
Member Author

@anuvratsingh Please do! If you need some guidance, you can post a comment here or in our discord :)

bors bot pushed a commit that referenced this issue Jul 6, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccessary.
--->

This Pull Request related to JsMap for #2098.

Any feedback on implementing JsMapIterator would be welcome. I wasn't entirely sure if it was the right approach, but as I worked on the example file, it felt like something at least similar would be needed to use Map's .entries(), .keys(), and .values() methods.

It changes the following:

- Implements JsMap Wrapper
- Implements JsMapIterator Wrapper
- Creates JsMap example in boa_examples
lameferret pushed a commit to lameferret/boa that referenced this issue Jul 6, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccessary.
--->

This Pull Request related to JsMap for boa-dev#2098.

Any feedback on implementing JsMapIterator would be welcome. I wasn't entirely sure if it was the right approach, but as I worked on the example file, it felt like something at least similar would be needed to use Map's .entries(), .keys(), and .values() methods.

It changes the following:

- Implements JsMap Wrapper
- Implements JsMapIterator Wrapper
- Creates JsMap example in boa_examples
@nekevss
Copy link
Member

nekevss commented Jul 7, 2022

@jedel1043 Would it be fine to work on JsArrayBuffer and JsDataView?

@jedel1043
Copy link
Member Author

@nekevss That would be very nice :D

@HalidOdat
Copy link
Member

HalidOdat commented Jul 7, 2022

Ummm... I didn't see this conversation before I implemented JsArrayBuffer (#2170 ) 😅 . I needed it to be implemented so I could fully fix (in a clean way) the Vec<u8> to JsTypedArray as discussed in #2058.

Though you can work on JsDataView if you still want it

@lameferret
Copy link
Contributor

Can I do JsDate, and why builtins::Math doesn't require a wrapper?

@HalidOdat
Copy link
Member

HalidOdat commented Jul 7, 2022

Can I do JsDate

Sure! :)

why builtins::Math doesn't require a wrapper?

The reasons is because it not a constructor and the static methods it provides do not offer anything that rust can't do (in fact they are wrappers around rust functions like f64.sqrt(), f64.pow(), etc)

bors bot pushed a commit that referenced this issue Jul 7, 2022
This PR adds a safe wrapper around JavaScript `JsSet` from `builtins::set`, and is being tracked at #2098.

Implements following methods 
- [x] `Set.prototype.size`
- [x] `Set.prototype.add(value)`
- [x] `Set.prototype.clear()`
- [x] `Set.prototype.delete(value)`
- [x] `Set.prototype.has(value)`
- [x] `Set.prototype.forEach(callbackFn[, thisArg])`
Implement wrapper for `builtins::set_iterator`, to be used by following.
- [x] `Set.prototype.values()`
- [x] `Set.prototype.keys()`
- [x] `Set.prototype.entries()`


*Note: Are there any other functions that should be added?

Also adds `set_create()` and made `get_size()` public in `builtins::set`.
@nekevss
Copy link
Member

nekevss commented Jul 7, 2022

Ummm... I didn't see this conversation before I implemented JsArrayBuffer (#2170 ) 😅

All good :) I can focus on working on JsDataView. I might look at adding some additional methods to JsArrayBuffer that were mentioned in #2058 like take and clone/borrow. Unless you don't think those are needed then I can leave it as is.

@HalidOdat
Copy link
Member

I might look at adding some additional methods to JsArrayBuffer that were mentioned in #2170 like take and clone/borrow. Unless you don't think those are needed then I can leave it as is.

As far as I know ArrayBuffer does not have such methods. The only method that is missing is ArrayBuffer.prototype.slice you can see JsArray::slice() implementation since they are very simular

@nekevss
Copy link
Member

nekevss commented Jul 8, 2022

Oh sorry! I linked the wrong thread. I updated the link in the above comment. The discussion had to do with providing methods to interact with array_buffer_data.

bors bot pushed a commit that referenced this issue Oct 2, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel necessary.
--->

This Pull Request is related to #2098 .

It changes the following:

- Implements a wrapper for `DataView`
- Adds an example of `JsDataView` to the `JsArrayBuffer` example file under boa_examples


Co-authored-by: jedel1043 <[email protected]>
@nekevss
Copy link
Member

nekevss commented Oct 4, 2022

@jedel1043 If nobody else is working it yet, I'll work on JsRegExp. Also, as a side note, I think these objects being moved into their own module is in a different commit, but is it going to be easier to separate these files into their own folder like external_objects or js_objects in object?

bors bot pushed a commit that referenced this issue Oct 15, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel necessary.
--->

This Pull Request is related to #2098.

It changes the following:

- Implements `JsRegExp`
- Adds a brief `JsRegExp` example under `boa_examples`
bors bot pushed a commit that referenced this issue Oct 26, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel necessary.
--->

This Pull Request is related to #2098.

It changes the following:

- Implements a wrapper for the `Generator` built-in object
- Adds to some of the documentation across the builtin wrappers with the goal of trying to clean up the documentation by making it a bit more consistent [on boa's docs](https://boa-dev.github.io/boa/doc/boa_engine/object/builtins/index.html)
bors bot pushed a commit that referenced this issue Nov 8, 2022
This PR adds a safe wrapper around JavaScript `JsDate` from `builtins::date`, and is being tracked at #2098.

#### Implements following methods 
- [x] `new Date()`
- [x] `Date.prototype.getDate()`
- [x] `Date.prototype.getDay()`
- [x] `Date.prototype.getFullYear()`
- [x] `Date.prototype.getHours()`
- [x] `Date.prototype.getMilliseconds()`
- [x] `Date.prototype.getMinutes()`
- [x] `Date.prototype.getMonth()`
- [x] `Date.prototype.getSeconds()`
- [x] `Date.prototype.getTime()`
- [x] `Date.prototype.getTimezoneOffset()`
- [x] `Date.prototype.getUTCDate()`
- [x] `Date.prototype.getUTCDay()`
- [x] `Date.prototype.getUTCFullYear()`
- [x] `Date.prototype.getUTCHours()`
- [x] `Date.prototype.getUTCMilliseconds()`
- [x] `Date.prototype.getUTCMinutes()`
- [x] `Date.prototype.getUTCMonth()`
- [x] `Date.prototype.getUTCSeconds()`
- [x] `Date.prototype.getYear()`
- [x] `Date.now()`
- [ ] `Date.parse()` Issue 4
- [x] `Date.prototype.setDate()`
- [x] `Date.prototype.setFullYear()`
- [ ] `Date.prototype.setHours()` Issue 3
- [x] `Date.prototype.setMilliseconds()`
- [ ] `Date.prototype.setMinutes()` Issue 3
- [x] `Date.prototype.setMonth()`
- [x] `Date.prototype.setSeconds()`
- [x] `Date.prototype.setTime()`
- [x] `Date.prototype.setUTCDate()`
- [x] `Date.prototype.setUTCFullYear()`
- [x] `Date.prototype.setUTCHours()`
- [x] `Date.prototype.setUTCMilliseconds()`
- [x] `Date.prototype.setUTCMinutes()`
- [x] `Date.prototype.setUTCMonth()`
- [x] `Date.prototype.setUTCSeconds()`
- [x] `Date.prototype.setYear()`
- [ ] `Date.prototype.toDateString()` Issue 5
- [ ] `Date.prototype.toGMTString()` Issue 5
- [ ] `Date.prototype.toISOString()` Issue 5
- [ ] `Date.prototype.toJSON()` Issue 5
- [ ] `Date.prototype.toLocaleDateString()` Issue 5 and 6
- [ ] `Date.prototype.toLocaleString()` Issue 5 and 6
- [ ] `Date.prototype.toLocaleTimeString()` Issue 5 and 6
- [ ] `Date.prototype.toString()` Issue 5
- [ ] `Date.prototype.toTimeString()` Issue 5
- [ ] `Date.prototype.toUTCString()` Issue 5
- [x] `Date.UTC()`
- [x] `Date.prototype.valueOf()`

### Issues
1. ~~`get_*()` and some other methods - They take `&self` as input internally, and internal struct shouldn't be used in a wrapper API. Therefore, these would require input to be `this: &JsValue, args: &[JsValue], context: &mut Context` like others and use `this_time_value()`?~~ Fixed using `this_time_value()`
2. ~~`to_string()`- how can I use `Date::to_string()` rather than `alloc::string::ToString`.~~ My bad it compiles, just `rust-analyzer` was showing it as an issue.
3. `set_hours()` and `set_minutes()` - they subtract local timezones when setting the value, e.g.
    - On further look:
    ```rust
       // both function call `builtins::date::mod.rs#L1038
      this.set_data(ObjectData::date(t));
      // `ObjectData::date` creates a new `Date` object `object::mods.rs#L423
      //                 | this date is chrono::Date<Tz(TimezoneOffset)> and Tz default is being used here which is GMT+0
      pub fn date(date: Date) -> Self {
        Self {
            kind: ObjectKind::Date(date),
            internal_methods: &ORDINARY_INTERNAL_METHODS,
        }
    }
     ```
     - BTW, in `object::mod.rs`'s `enum ObjectKind` there is `Date(chrono::Date)` and it requires 
     the generic argument, how is it being bypassed here?
     - Also in `set_minutes()` step 6, `LocalTime` should be used.
```rust
// reference date = 2000-01-01T06:26:53.984
date.set_hours(&[23.into(), 23.into(), 23.into(), 23.into()], context)?;
// would add tiemzone(+5:30) to it
// Is                 2000-01-01T17:53:23.023
// Should be    2000-01-01T23:23:23.023
```
4. `parse()` - it uses `chrono::parse_from_rfc3339` internally, while es6 spec recommends ISO8601. And it can also parse other formats like from [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse) `04 Dec 1995 00:12:00 GMT` which fails. So what should be done about it.
5. `to_*()` - This is more general, as the internal date object uses `chrono::NaiveDateTime` which doesn't have timezone. It doesn't account for `+4:00` in example below.
```rust
// Creates new `Date` object from given rfc3339 string.
let date = JsDate::new_from_parse(&JsValue::new("2018-01-26T18:30:09.453+04:00"), context);
println!("to_string: {:?}", date2.to_string(context)?); 
// IS:          Sat Jan 27 2018 00:00:09 GMT+0530
// Should:  Fri Jan 26 2018 20:00:09 GMT+0530
```
6. `to_locale_*()` - requires [`ToDateTimeOptions`](https://402.ecma-international.org/9.0/#sec-todatetimeoptions) and localization would require chrono's `unstable-locales` feature, which is available for `DateTime` and not for `NaiveDateTime`. 
    - I should have looked properly, `to_date_time_options` is already implemented in `builtins::intl`. Anyway, I would still need some tips on how to use it. What would function signature be like in wrapper API, how would `options` be passed to the said API. 
    - So `to_date_time_options()` takes `options: &JsValue` as an argument and build an object from it and fetch properties through `Object.get()`. If I want `options` to be `{ weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' }` what would `JsValue` look like to make it all work. 
    ```rust 
    date.to_locale_date_string(&[JsValue::new("en_EN"), OPTIONS], context)?;
    // OPTIONS need to be a JsValue which when converted into an object 
    // have these properties { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' };
    ```


### Possible improvements 
1. Right now, `object::jsdate::set_full_year()` and alike (input is a slice) are like below, `into()` doesn't feel ergonomic.
    ```rust
    #[inline]
    pub fn set_full_year(&self, values: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
        Date::set_full_year(&self.inner.clone().into(), values, context)
    }
    // Usage 
    date.set_full_year(&[2000.into(), 0.into(), 1.into()], context)?;
    // How can something like this be made to work
    #[inline]
    pub fn set_full_year<T>(&self, values: &[T], context: &mut Context) -> JsResult<JsValue>
    where
        T: Into<JsValue>,
    {
                                                          | expected reference `&[value::JsValue]` 
                                                          | found reference `&[T]`        
        Date::set_full_year(&self.inner.clone().into(), values, context)
    }
    ```
2. Any other suggestion?
@nekevss
Copy link
Member

nekevss commented Dec 13, 2022

Awkward, accidentally opened up an issue 😂 Just wanted to note that JsGenerator was added with #2380.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API builtins PRs and Issues related to builtins/intrinsics E-Medium Medium difficulty problem enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants