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

Node.js API cleanup ideas for feedback #9766

Closed
brodycj opened this issue Nov 23, 2016 · 4 comments
Closed

Node.js API cleanup ideas for feedback #9766

brodycj opened this issue Nov 23, 2016 · 4 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.

Comments

@brodycj
Copy link

brodycj commented Nov 23, 2016

  • Version: 6.9.1/7.2.0/general
  • Platform: UNIX/general
  • Subsystem: src (general)

Considering that the node.h header is exported for both addons and embedders I see a few things that could be improved:

Will discuss the remaining ideas elsewhere (related to nodejs/api#31):

  • Break node.h into the smaller parts such as:
    • API for addons
    • external API for embedder(s)
    • type definitions

Other ideas:

  • Keep the definition of node_module internal
  • C API for Node embedders

I would be happy to submit PRs for whatever ideas may be desired by the community.

@italoacasas italoacasas added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 23, 2016
@Fishrock123
Copy link
Contributor

C API for Node embedders

Please see nodejs/api#31 :)

@Fishrock123
Copy link
Contributor

cc @nodejs/api @nodejs/addon-api

@bnoordhuis
Copy link
Member

Cleanup or remove lines to define size_t/ssize_t for UNIX/Windows [...] Ideal may be node-specific size typedefs.

I think we can simply replace that with a #include <stddef.h> because it's a workaround for old Visual Studio versions that we no longer support. No need for custom typedefs.

Keep the definition of node_module internal

Can you go into more detail?

@brodycj
Copy link
Author

brodycj commented Nov 23, 2016

Keep the definition of node_module internal
Can you go into more detail?

See PR #9767

replace that with a #include <stddef.h>

node.h includes v8.h which in turn includes stddef.h - PR #9767 is now updated to take the lines out - I hope this is good enough.

C API for Node embedders
Please see nodejs/api#31 :)

Thanks @Fishrock123!

Fixes are in PR #9767, other changes will be discussed in API/EPS issues. Closing this one, looking forward to getting #9767 reviewed.

@brodycj brodycj closed this as completed Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

No branches or pull requests

4 participants