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

Plain data objects #493

Merged
merged 19 commits into from
May 14, 2020
Merged

Plain data objects #493

merged 19 commits into from
May 14, 2020

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Feb 21, 2020

Implements these 'plain object' operations:

  • creating an empty object
  • getting and setting properties, named properties, index properties
  • listing properties

And the required tag checks to make that work, and the required array methods to make get_own_property_names usable.

Most of it is quite straightforward, just calling napi functions. The remaining changes are about passing Env values to functions that need it under n-api, but did not need it under nan. The test for the object code uses downcasting, and the downcasting and typecheck code did not previously receive a reference to the Env. The js_array.len() function also did not receive a reference to the Env, but needs one to eventually call the napi function.

For the js_array.len() function, which is used by other methods on the impl, I added a private len_inner() function with a consistent API to save some feature flag checks. It could be removed if the nan implementation is removed later.

@goto-bus-stop goto-bus-stop mentioned this pull request Mar 27, 2020
2 tasks
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This looks great! This is really exciting. I just asked for a couple of clarifying comments in some of the subtler bits of the implementation.

crates/neon-runtime/src/napi/array.rs Show resolved Hide resolved
crates/neon-runtime/src/napi/object.rs Show resolved Hide resolved
crates/neon-runtime/src/napi/object.rs Show resolved Hide resolved
crates/neon-runtime/src/napi/tag.rs Show resolved Hide resolved
@goto-bus-stop
Copy link
Member Author

Test failure is because Node.js 10 appears to return a Number in a mixed get_own_property_names result ([0, "a", "whatever"] in the test case), while Node.js 12 casts them all to strings (["0", "a", "whatever"]). nodejs/node#27524

Object.getOwnPropertyNames() returns only strings and the n-api documentation also claims it returns an array of strings, so I'll add some code to convert numeric indices to strings in those older node versions.

@goto-bus-stop
Copy link
Member Author

goto-bus-stop commented Apr 8, 2020

There was another complication: get_property_names includes inherited names, so I also added some has_own_property checks. That required one of two things:

  • Removing elements from the array
  • Creating a second array that only includes own properties

I tried the first approach using napi_delete_property, but that just resulted in holey arrays; I think I'd have to call the .splice() method from Rust to make that work and that is kind of difficult (and might not be very fast…).
So, I opted for the second.

I did work on a test for it too, but it requires a way to call into Rust functions from JS, which isn't yet implemented in this PR. I rebased #507 and added a test there.

One final thing to check: does get_property_names include Symbol properties? Its name suggests no, but let's find out 😂
e; It does not include Symbol properties, so that's good! 🎉

@goto-bus-stop goto-bus-stop requested a review from dherman April 8, 2020 15:04
I thought I could be clever and micro-optimize this a bit, but
`has_own_property` really does require a string argument, so we have to
do the conversion first.
@dherman
Copy link
Collaborator

dherman commented Apr 9, 2020

@goto-bus-stop I'll review more closely in a bit, thanks for updating!

Technically, the semantics of getOwnPropertyNames isn't the same as doing a full enumeration and then filtering out anything that isn't an own property. For example, a proxy could behave very differently.

But it looks like N-API simply doesn't expose the getOwnPropertyNames operation, so this might be the closest we can come to matching the right behavior for now. I've asked some Node and N-API leaders how we might propose a new N-API function to do what we need.

@dherman
Copy link
Collaborator

dherman commented Apr 9, 2020

Per the replies on Twitter, I've filed an N-API issue proposing an API.

@dherman
Copy link
Collaborator

dherman commented Apr 10, 2020

@goto-bus-stop Oh, wait! Twitter came to the rescue and explained that napi_get_all_property_names() is the API we need here. You can pass a key_mode value of napi_key_own_only and a key_filter of napi_key_skip_symbols. That will have exactly the behavior of Object.getOwnPropertyNames().

@goto-bus-stop
Copy link
Member Author

Riiight Proxies…I really should've read the spec instead of guessing!

I wanted to use napi_get_all_property_names first, but the documentation said it's only supported in Node.js 13+, which was a dealbreaker. However, looking at Node.js commit history now, it looks like it's actually been backported to 10.x too, so that makes things easier 😅
Just needs a nodejs-sys update to add the newer APIs.

@goto-bus-stop
Copy link
Member Author

goto-bus-stop commented Apr 10, 2020

Oh lol—the backport was released in 10.20.0 only 2 days ago, which is why I didn't know about it yet :) Then we also shouldn't need the whole convert-to-string dance anymore.

@goto-bus-stop
Copy link
Member Author

goto-bus-stop commented Apr 10, 2020

napi_get_all_property_names is not yet in Node.js 12.x but may be backported in the next minor in April per nodejs/node#30006 (comment) is being added in nodejs/node#32482

@goto-bus-stop
Copy link
Member Author

I guess tests won't pass on Node 12 until a new minor is released. Should we in the mean time just rely on my buggy implementation, and file a follow-up issue to upgrade as soon as a Node.js 12.x with the backport is available?

@dherman
Copy link
Collaborator

dherman commented Apr 10, 2020

@goto-bus-stop Thanks for all this detective work! I think your workaround is totally fine for now until the new API is backported to 12. Do you mind also filing a followup issue so we can replace the workaround once 12 is ready?

@goto-bus-stop
Copy link
Member Author

filed follow up in #511

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much for all the investigations and revisions you've done. :shipit:

crates/neon-runtime/src/napi/array.rs Show resolved Hide resolved
@dherman dherman merged commit b2d3514 into neon-bindings:master May 14, 2020
@goto-bus-stop goto-bus-stop deleted the objects branch May 14, 2020 22:55
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.

2 participants