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

Remove JSNativeApi.cs #187

Merged
merged 13 commits into from
Jan 27, 2024
Merged

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Dec 19, 2023

This PR is a part of the code refactoring that aims to simplify the code and to improve public API.
In this PR we remove the JSNativeApi.cs file that mostly had extensions for the JSValue struct.
We still have JSNativeApi class in the JSError.cs file that we can remove in future PR.
The key parts of this PR:

  • JSVativeApi.cs code is moved inside of the JSValue struct.
  • Code that access JSRuntime functions in JSValue is changed to minimize required verifications. New private functions
    GetRuntime and GetCurrentRuntime return napi_env and napi_value in one call.
  • Code that was dependent on the JSNativeApi is changed to use the new JSValue functions.
  • JSMarshaller changes reflect the function changes from JSNativeApi static methods to JSValue instance methods.
  • Some comment and spelling fixes.

In the next PRs we should improve the JSValue API and API for other classes and structs.
E.g. we can move some methods to JS type specific structs.

@vmoroz vmoroz requested a review from jasongin December 19, 2023 21:53
Copy link
Member

@jasongin jasongin left a comment

Choose a reason for hiding this comment

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

Overall this is a great improvement to the public API surface.

I just have some concerns about keeping conversion APIs for JSBigInt consistent with other types.

src/NodeApi/JSBigInt.cs Outdated Show resolved Hide resolved
src/NodeApi/JSBigInt.cs Show resolved Hide resolved
src/NodeApi/JSBigInt.cs Outdated Show resolved Hide resolved
src/NodeApi/JSError.cs Outdated Show resolved Hide resolved
src/NodeApi/JSValue.cs Outdated Show resolved Hide resolved
src/NodeApi/JSValue.cs Outdated Show resolved Hide resolved
This was referenced Jan 19, 2024
@vmoroz vmoroz merged commit 1b708bf into microsoft:main Jan 27, 2024
7 checks passed
@vmoroz vmoroz deleted the PR/remove_jsnativeapi_cs branch January 27, 2024 05:01
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