Skip to content

Initial scaffolding for the project. #1

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
May 10, 2021
Merged

Conversation

NickNaso
Copy link
Member

@NickNaso NickNaso commented Jan 22, 2021

This PR has the purpose to provide a first implementation for the project.
The header files are placed in a folder called include. I just copied the files:

  • js_native_api_types.h
  • js_native_api.h
  • node_api_types.h
  • node_api.h

My idea about is to automate the process of copying the file from the main Node.js repo to this repo. Essentially we could check if the header files on the Node.js repo has been changed and in that case copy them to this repo.
What do you think about that?
The index.js files expose the include path for the header files and the list of all symbols exported by Node-API.

@@ -0,0 +1,148 @@
'use strict'

Choose a reason for hiding this comment

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

@NickNaso can you please explain under what circumstances a user of this repo would need the list of symbols exposed as a JS array? Also, if we do need them, would it be possible to have two arrays? One for symbols from js_native_api.h and one for those from node_api.h?

Copy link
Member

Choose a reason for hiding this comment

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

+1. I think explaining why they are there and how they can be used would be good. I also wonder if we need to somehow group them by what is available in each NODE-API version?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gabrielschulhof my idea is to give the possibility to use the module in a build process or using it in other module that needs to know the inclusion path or the list of the symbols exported:

Example of usage of the exported symbols on a build process:

node -p "require('./index').symbols.join('\n')" > node_api.def
dlltool -d node_api.def -y libnode.a

Example of usage in other JacaScript module:

'use strict'
const headers = require('node-api-headers')
// Do something

Yes it's possible to provide the list of symbols for:

  • js_native_api.h
  • node_api.h

My idea is to change the symbols.js in something like this:

'use strict'

const js_native_api_symbols = [
    // Here the list of the symbols for js_native_api.h
]

const node_api = [
     ...js_native_api_symbols,
    // Here the list of symbols for node_api.h
]

module.exports = {
    js_native_api_symblos,
    node_api_symbols
}

For me the symbol exported by js_native_api_symbols should be considered a subset of those exported by node_api.h

Copy link
Member Author

Choose a reason for hiding this comment

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

@mhdawson It's my intentio explain the useage in documentation after we end this first step on implementation. We could group the symbols by Node-API version so in that case we should expose them like:

'use strict'

module.exports = {
    V1: {
        js_native_api_symblos: [
            // List of symbols in js_native_api.h for a specific version
        ],
        node_api_symblos: [
          // List of symbloas in node_api.h for a specific version
        ]
    },
   // ...
}

I think that list the symblos by version is a better approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could group the symblols considering the experimental cases like below:

'use strict'

module.exports = {
    V1: {
        js_native_api_symblos: [
            // List of symbols in js_native_api.h for a specific version
        ],
        node_api_symblos: [
          // List of symbols in node_api.h for a specific version
        ],
        experimental: [
         // List of symbols for experimental features
       ]
    },
   // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

I really think we should exclude the experimental ones. I think it will just come back to haunt us if we include them because they change without an associated version number. In the example above, there may be experimental symbols that were in some Node.js versions that support V1, but not in others so it would not necessarily be correct to included them in the experimental list for V1. As another example an experimental symbol could be experimental during both the timeframe of V1 and V2, so might need to appear in both. It's just too complicated to get right in my opinion....

const char* release;
} napi_node_version;

#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we'd remove the parts guarded by NAPI_EXPERIMENTAL


#endif // NAPI_VERSION >= 4

#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we'd remove the parts guarded by NAPI_EXPERIMENTAL

} napi_key_conversion;
#endif // NAPI_VERSION >= 6

#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we'd remove the parts guarded by NAPI_EXPERIMENTAL

bool* result);
#endif // NAPI_VERSION >= 7

#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we'd remove the parts guarded by NAPI_EXPERIMENTAL

#include <stddef.h> // NOLINT(modernize-deprecated-headers)
#include <stdbool.h> // NOLINT(modernize-deprecated-headers)

// Use INT_MAX, this should only be consumed by the pre-processor anyway.
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we'd remove the parts guarded by NAPI_EXPERIMENTAL

@mhdawson
Copy link
Member

In terms of:

My idea about is to automate the process of copying the file from the main Node.js repo to this repo. Essentially we could check if the header files on the Node.js repo has been changed and in that case copy them to this repo.
What do you think about that?

I think we should only update when there is a new version of the NODE-API. Having script to automate that sounds good, but I think we only want new versions when there is a new version of NODE-API, for example adding version 8. This is related to my comment about stripping out the Experimental sections as those will potentially change and I think that adds more complications than we want to try to cope with in providing the headers.

@jschlight
Copy link

When we've settled on how we want this to work, I'm happy to take a shot at the GitHub Actions to implement it. They can be triggered on number of different events such as a merge to main.

@NickNaso
Copy link
Member Author

In terms of:

My idea about is to automate the process of copying the file from the main Node.js repo to this repo. Essentially we could check if the header files on the Node.js repo has been changed and in that case copy them to this repo.
What do you think about that?

I think we should only update when there is a new version of the NODE-API. Having script to automate that sounds good, but I think we only want new versions when there is a new version of NODE-API, for example adding version 8. This is related to my comment about stripping out the Experimental sections as those will potentially change and I think that adds more complications than we want to try to cope with in providing the headers.

@mhdawson I agree with you we need to identify the changes on the main Node.js repo and check if those changes have produced a new version for the Node-API, in that case we should copy the files and create a new release for the node-api-headers.

@mhdawson
Copy link
Member

@NickNaso are there still changes to be made based on the discussion in the issue?

@NickNaso
Copy link
Member Author

@NickNaso are there still changes to be made based on the discussion in the issue?

Hi @mhdawson,
I need to update the PR. I try to finish the work in the next days. Sorry for the delay.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented May 7, 2021

@KevinEady this look good to you?

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhdawson mhdawson merged commit 15b9d14 into nodejs:main May 10, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants