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

Add napi crosswalk #398

Closed
wants to merge 1 commit into from
Closed

Add napi crosswalk #398

wants to merge 1 commit into from

Conversation

springmeyer
Copy link
Contributor

@springmeyer springmeyer commented Jun 24, 2018

This crosswalk document tracks when the NAPI version is added or bumped in node core (e.g. nodejs/node@6572f63).

The reason I'm creating it is that I'm trying to figure out a way to support both node versions without NAPI support and multiple NAPI versions (in those node versions that do support NAPI). E.g. what if you want to make binaries for:

  • node v4.0.x (without N-API)
  • node v6.0.x (without N-API)
  • Node v6, v8, etc that are recent enough minor versions to support N-API

@jschlight - does this crosswalk look correct to you as a starting point to figure out why node versions support N-API? Also @jschlight can you explain why these lines are present and what they do?

if (!version) { // this code should never need to be updated
if (versionArray[0] === 9 && versionArray[1] >= 3) version = 2; // 9.3.0+
else if (versionArray[0] === 8) version = 1; // 8.0.0+
}
. Do they mean to catch node versions that have the napi but just don't declare in via process.versions.napi?

@jschlight
Copy link
Contributor

@springmeyer The napi_crosswalk.json file looks right to me. You may account for them elsewhere, but I believe Node 9.3.0 and 9.4.0 also support N-API version 2.
The code you call out above is, as you mention, meant to determine the N-API version for those older versions of Node that did not implement process.versions.napi.

@springmeyer
Copy link
Contributor Author

@jschlight thanks for sharing.

meant to determine the N-API version for those older versions of Node that did not implement process.versions.napi.

So, my script uses the #define NAPI_VERSION in node_version.h to indicate which N-API version is supported per node version. Is this not the best indicator? If there are versions of node that supported N-API without declaring it, how can I detect that?

Also, another 2 questions for you:

  1. If a developer uses "napi_versions" in the package.json for node-pre-gyp, is there any way (with node-pre-gyp) to also support node versions that don't support N-API at all? Or is the design of your node-pre-gyp support of "napi_versions" intended to only work with the most recent versions of node (and not say node v4 or versions of node v6 or v6 that pre-dated N-API support?)

  2. Per my above question I tried using {node_abi_napi}, which allows you to target either the node abi (for version of node that don't support N-API) or N-API. But then, as far as I can tell, I'm locked into only targeting N-API version 1. Do you see what I'm getting at? Is there a combination of N-API related variables you added which could be used to support both version of node by node-abi (that don't have N-API support) and multiple N-API versions? Or is that a complexity you did not anticipate or thing was needed to support?

@springmeyer
Copy link
Contributor Author

@jschlight I'll also mention that my confusions/questions are coming from trying to get TryGhost/node-sqlite3#1008 working. It seems like I have two options:

  • Release a new major version of node-sqlite3 that only supports the versions of node that support N-API
  • Release a new minor version of node-sqlite3 that provides binaries for both non N-API versions and N-API versions.

It is the non N-API versions issue that I'm supper fuzzy on, since I notice that I can build TryGhost/node-sqlite3#1008 with node v4 if I use {napi_build_version} instead:

diff --git a/package.json b/package.json
index a7a6e1b..cd0e6d5 100644
--- a/package.json
+++ b/package.json
@@ -9,13 +9,10 @@
   },
   "binary": {
     "module_name": "node_sqlite3",
-    "module_path": "./lib/binding/napi-v{napi_build_version}",
+    "module_path": "./lib/binding/{node_abi_napi}",
     "host": "https://mapbox-node-binary.s3.amazonaws.com",
     "remote_path": "./{name}/v{version}/{configuration}/{toolset}/",
-    "package_name": "{platform}-{arch}-napi-v{napi_build_version}.tar.gz",
-    "napi_versions": [
-      1
-    ]
+    "package_name": "{platform}-{arch}-napi-v{node_abi_napi}.tar.gz"
   },
   "contributors": [
     "Konstantin Käfer <[email protected]>",

Context: breaking node v4 support for node-sqlite3 I think could cause a lot of user pain if done soon (since we're not far out of LTS support for node v4).

@jschlight
Copy link
Contributor

@springmeyer N-API was introduced in Node 8.0.0 and was in experimental status until Node 10.0.0 when it left experimental status. While in experimental status, the N-API implementation evolved. This means that every version of Node 8, 9, and 10 supports some version of N-API.

Every version of Node that supports N-API includes a #define NAPI_VERSION in the source code which is a reliable indication of the N-API version. In earlier versions, when N-API was still in experimental status, the #define NAPI_VERSION was in the node_api.cc file. Before leaving experimental status, the #define NAPI_VERSION was moved to the node_version.h file where it will remain indefinitely. The #define NAPI_VERSION was moved in order to support process.versions.napi which exposes the N-API version to JavaScript.

The current N-API implementation has since been back-ported to Node 6, currently in experimental status, starting with Node 6.14.2.

When I first made the modifications to node-pre-gyp to support N-API, it did not occur to me that module maintainers would need to build simultaneously against N-API and NAN. In hindsight, however, I can see that this is an essential need. There has been some discussion of this issue here and here. I need to investigate how the proposed solution might also apply to node-pre-gyp. I've scheduled time on Friday to work in depth on node-pre-gyp. I hope I'll have a better understanding of this issue then, including the role of {node_abi_napi}. I'd like to see if we can agree on a straightforward solution that will reliably address this issue.

@springmeyer
Copy link
Contributor Author

Thanks for the thoughts and extra links @jschlight.

@jschlight
Copy link
Contributor

@springmeyer I've posted a proposed solution for building node-sqlite3 at #402. Please let me know if you feel it can work.

@springmeyer springmeyer closed this Dec 3, 2018
@springmeyer springmeyer deleted the napi-meta branch December 3, 2018 02:17
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