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

[builtins - Object] Object.defineProperties() not implemented #552

Closed
croraf opened this issue Jul 8, 2020 · 11 comments · Fixed by #746
Closed

[builtins - Object] Object.defineProperties() not implemented #552

croraf opened this issue Jul 8, 2020 · 11 comments · Fixed by #746
Assignees
Labels
builtins PRs and Issues related to builtins/intrinsics E-Easy Easy enhancement New feature or request good first issue Good for newcomers Hacktoberfest Hacktoberfest 2021 - https://hacktoberfest.digitalocean.com

Comments

@croraf
Copy link
Contributor

croraf commented Jul 8, 2020

Also, as per: #543 (comment) , after this is done, Object.create should be improved to handle second argument (the object properties) along with some tests.

@croraf croraf added the bug Something isn't working label Jul 8, 2020
@Razican Razican added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request and removed bug Something isn't working labels Jul 8, 2020
@Razican
Copy link
Member

Razican commented Jul 8, 2020

Hi, this seems to be a new enhancement request, not something that is buggy. Could you please fill the template we have for this? There are no links to the spec or proper explanations on what this means.

@croraf
Copy link
Contributor Author

croraf commented Jul 8, 2020

Which explaination you need on "Object.defineProperties() not implemented"?

image
It is not clear which of these should be selected in this case. Perhaps "Feature request" description should be improved.

@croraf croraf changed the title [builtins - Object] Object.defineProperties() not implemented [builtins - Object] Object.defineProperties() not working Jul 8, 2020
@croraf croraf changed the title [builtins - Object] Object.defineProperties() not working [builtins - Object] Object.defineProperties() not implemented Jul 8, 2020
@Razican
Copy link
Member

Razican commented Jul 8, 2020

It is not clear which of these should be selected in this case. Perhaps "Feature request" description should be improved.

Done in #554.

Which explaination you need on "Object.defineProperties() not implemented"?

Do you have a link to the spec where this is explained? A code example that you would like to make work?

Also, you mention Object.create. What is this second parameter? Do you have a link to the spec? An example code? You mention we should add new tests. Which tests? why?

@croraf
Copy link
Contributor Author

croraf commented Jul 8, 2020

Do you have a link to the spec where this is explained? A code example that you would like to make work?

It should not be my job as a end-user and issue reported to suggest the way how to implement it, and provide suggestions where to find the information. The reporter should have a discretion to add this or not. The issue report is perfectly clear as is.

My extra effort here was that I checked that Object.defineProperties does not work because it is not implemented. I could have just said it is not working as in Chrome, so it would be a bug. It is not the mandatory job of the reported to search if some JS code does not work because "it is not implemented", or because "it is wrongly implemented".

Also, you mention Object.create. What is this second parameter? Do you have a link to the spec? An example code? You mention we should add new tests. Which tests? why?

Although I think this is also clear, I added a link to the related closed issue which provides more details.

@Razican
Copy link
Member

Razican commented Jul 8, 2020

It should not be my job as a end-user and issue reported to suggest the way how to implement it, and provide suggestions where to find the information. The reporter should have a discretion to add this or not. The issue report is perfectly clear as is.

Sure, but understand that the more detail you give, the more possibilities you have for us to take this into account.

In any case, let me add some information for future developers.

This is defined in the spec here. This should be added to the Object global object in this file, similarly to the Object.create() method.

Then, this needs to be added as a property of the Object in the end of the file, and we could take the opportunity to fix the Object.create() method above by taking into account the second parameter, instead of panicking.

Note that this Object.defineProperties() will require first calling on get_type() to assert the type of the object, but in fact, this can be a bit more efficient by doing an if let in the Value.

Then, in the spec, you call ToObject(), which is defined in the interpreter here.

Then it calls ToPropertyDescriptor() and DefinePropertyOrThrow(), which I think they are not defined. And in fact, we might not be properly using property descriptors. Maybe @HalidOdat or @jasonwilliams can give a hint.

@Razican Razican added the good first issue Good for newcomers label Jul 8, 2020
@HalidOdat
Copy link
Member

Then it calls ToPropertyDescriptor() and DefinePropertyOrThrow(), which I think they are not defined. And in fact, we might not be properly using property descriptor. Maybe @HalidOdat or @jasonwilliams can give a hint.

Yes. ToPropertyDescriptor() and DefinePropertyOrThrow() are not implemented, I planed to fix/implement (the internal methods are not spec compliant) them after #553

@jasonwilliams jasonwilliams added E-Easy Easy Hacktoberfest Hacktoberfest 2021 - https://hacktoberfest.digitalocean.com labels Sep 30, 2020
@jasonwilliams
Copy link
Member

Now that the blockers are out the way this should be straight forward for someone to pick up

@dvtkrlbs
Copy link

i can take this issue

@dvtkrlbs
Copy link

Ok i am creating the Object.defineProperties function but i did not understand something in the spec.
Let keys be ? props.[[OwnPropertyKeys]](). what this means and is it defined already in the source code. I see these things are defined here but i dont understand if i should define them myself.

@croraf
Copy link
Contributor Author

croraf commented Sep 30, 2020

@dvtkrlbs I guess that should be a method of Rust's Object structure that seems not to be implemented, so should be implemented within this task or before this task. (Also you should join discord channel for boa for easier discussion. Link should be on the homepage)

@jasonwilliams
Copy link
Member

Ok i am creating the Object.defineProperties function but i did not understand something in the spec.
Let keys be ? props.[[OwnPropertyKeys]](). what this means and is it defined already in the source code. I see these things are defined here but i dont understand if i should define them myself.

I imagine your "defineProperties" will be similar to this:
https://github.com/boa-dev/boa/blob/master/boa/src/builtins/object/mod.rs#L98-L105

Let keys be ? props.[OwnPropertyKeys].
This means loop through each key in the object's property list.
The ? just means each access shouldn't result in an abrupt ending

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics E-Easy Easy enhancement New feature or request good first issue Good for newcomers Hacktoberfest Hacktoberfest 2021 - https://hacktoberfest.digitalocean.com
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants