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

Switch from NAN to N-API #1304

Merged
merged 29 commits into from
Apr 27, 2020
Merged

Switch from NAN to N-API #1304

merged 29 commits into from
Apr 27, 2020

Conversation

mohd-akram
Copy link
Contributor

Closes #830.

@springmeyer
Copy link
Contributor

Thanks @mohd-akram - this builds and all tests pass for me (I'm using OS X 10.15.3). I notice that travis is not running any of the jobs for mac and linux which is odd - not sure why that is. And of course some electron builds are failing on windows.

@springmeyer
Copy link
Contributor

looks like travis builds stopped working when #1294 was merged. /cc @kewde

@jschlight
Copy link
Collaborator

I very much appreciate the progress @mohd-akram has made on this.

The N-API Version Matrix from the Node.js documentation shows that N-API version 3, which is the recommended version, is not supported on Node.js versions 4, 5, and 7. Removing those versions from the CI files will eliminate some of the build errors.

I'm happy to make these changes, but the process to update another contributor's PR is not clear to me.

@springmeyer
Copy link
Contributor

I'm happy to make these changes, but the process to update another contributor's PR is not clear to me.

Same here! I was surprised to see my commit show up above (as I was trying to push to my own branch).

Anyway, how about we take the unique approach of all working on the same PR here to try to drive this to a quick conclusion?

the Node.js documentation shows that N-API version 3, which is the recommended version, is not supported on Node.js versions 4, 5, and 7. Removing those versions from the CI files will eliminate some of the build errors.

I'm supportive of this 👍 Please commit this change @jschlight

@springmeyer
Copy link
Contributor

Please commit this change @jschlight

@jschlight ping me via email if you need access

@jschlight
Copy link
Collaborator

It's really good that we can now have a single code-base to work with.

I've been working with this code for the past couple of weeks attempting to understand the test failures reported by Travis CI and AppVeyor. I've been stymied in this effort by some of the test failures not being reproducible.

The commit above makes a change to the first two Mocha tests. With the above change, the tests run successfully on my macOS development system under Node v10.20.1. However, they consistently fail under Node v12.16.2.

To this point I've been unsuccessful in determining the cause of the error due the limited
time I currently have available. The error seems to be related to differences between the libuv and N-API threading models. The code looks correct to me which leads me to believe the issue may be a change in Node.js itself.

If we can get a clearer, specific understanding of why this particular test is failing, I can work with other members of the N-API team to help get it resolved.

@mohd-akram
Copy link
Contributor Author

I think I've managed to fix the issue. Tests are passing now. Electron versions < 3 fail to build with this error:

../src/statement.h:176:13: error: use of undeclared identifier
      'napi_get_uv_event_loop'
            napi_get_uv_event_loop(stmt->Env(), &loop);

@jschlight
Copy link
Collaborator

napi_get_uv_event_loop was added in N-API version 2. This compile error would occur when compiling against N-API version 1. I've added a define for NAPI_VERSION=3 to the binding.gyp file. We'll see if this addresses the issue in the CI.

@springmeyer
Copy link
Contributor

Thanks @jschlight and @mohd-akram! Great progress here!

One though I've not seen discussed yet: which electron versions to keep building for. I think if we bump the major version when this is released we should also remove some of the older electron versions. @kewde can you advise which versions must be kept?

@jschlight
Copy link
Collaborator

It looks like the builds for the older Electron versions are still failing, but now for new reasons. napi_async_context is undefined in Travis CI and napi_callback_scope is undefined in AppVeyor. The Electron builds are a complete mystery to me.

I've updated the package.json file to support N-API builds. I'm not confident it will address the Electron build issues, but it needs to be updated anyway.

@mohd-akram
Copy link
Contributor Author

We shouldn't define NAPI_VERSION in binding.gyp - it's defined in node_version.h. The electron builds are failing because they are missing napi_get_uv_event_loop in their headers (see https://electronjs.org/headers/v2.0.1/node-v2.0.1.tar.gz). In the case of Windows, even if we add a declaration for it manually, it's still missing from the .lib files and so linking fails. If we will need to continue supporting older versions of Electron, we can try https://github.com/node-ffi-napi/get-uv-event-loop-napi-h.

@mohd-akram mohd-akram marked this pull request as ready for review April 18, 2020 14:44
@jschlight
Copy link
Collaborator

Based on the comment above from @mohd-akram, I can now see why the old Electron builds are failing. I downloaded and unpacked https://electronjs.org/headers/v2.0.1/node-v2.0.1.tar.gz. Examining node_version.h shows that Electron 2.0.1 is bundling Node.js 8.9.3 which does not support N-API. Electron 1.8.4 bundles Node.js 8.2.1 which also does not support N-API. However, Electron 3.0.0 bundles Node 10.2.0 which does support N-API. This explains what @mohd-akram reports above.

So the move to N-API precludes the use of Electron versions prior to 3.0.0.

@kewde
Copy link
Collaborator

kewde commented Apr 19, 2020

Thank you for the contributions @mohd-akram && @jschlight.

With regards to the minimal requirements, we will be deprecating support for any version below:

Node v10 will be the lowest version it should be able to work with as it still receives maintenance.
Electron v6 (which is basically Node v12 so don't worry), maintenance.

@kewde
Copy link
Collaborator

kewde commented Apr 19, 2020

I've reduced the build matrix on the CI to just the required versions.

Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

I see this is now passing on travis, so 👍 from me to merge this.

But let's also get a 👍 review from @kewde as well.

Thanks everyone!

@kewde
Copy link
Collaborator

kewde commented Apr 21, 2020

Since N-API is ABI stable, an add-on binary built for N-API version 3 will run, unchanged, on all versions of Node that support N-API version 3 or later.

Hmm, this is interesting. We could perhaps add a single job (for each architecture: linux, macos, win32 & win64) to the Travis CI responsible pushing the binary releases.

That way we only have to wait for 4 jobs to be completed when pushing a binary release.

@xPaw
Copy link
Contributor

xPaw commented Apr 24, 2020

Node v14 should be added to the test matrix.

v11 is already EOL, and v13 will be EOL in June.

https://github.com/nodejs/Release

@kewde kewde merged commit 0ffea2d into TryGhost:master Apr 27, 2020
@kewde
Copy link
Collaborator

kewde commented Apr 27, 2020

Thank you to everyone involved, in particular @jschlight @mohd-akram !
I can not express enough gratitude for your contributions.
Hope everyone is glad that everything finally got upstreamed.

@theogravity
Copy link

theogravity commented Jul 1, 2020

@kewde can you consider making @mohd-akram a core contrib + npm publish (if he's up for it) to help reduce your load? It's clear he understands the code base and has made a few major contribs so far.

@kewde
Copy link
Collaborator

kewde commented Jul 2, 2020

@theogravity cc @mapsam @springmeyer

@springmeyer
Copy link
Contributor

springmeyer commented Jul 2, 2020

👍 agree on adding @mohd-akram, if interested. To make this easiest (@kewde being able to add/encourage more collaborators) I think we should consider moving this repo to its own org outside Mapbox - that way (as the original maintainer) I can give full maintainer-ship to @kewde and he can pass it to others if interested. I can look into getting approval from Mapbox to do that if others are interested. It makes sense to me since no Mapbox employees are actively developing on this module anymore, but it is important to the node community to have it maintained by those who are using it and have ideas to improve it.

@mohd-akram
Copy link
Contributor Author

Hi folks. Thank you for considering me for being a core contributor. My initial PR, built on top of the work done by @jschlight, was motivated by getting worker threads working with node-sqlite3 for a project. I'm no longer working on that project, but I do use node-sqlite3 in a few other projects and would be happy to assist.

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.

N-API Support for node-sqlite3
6 participants