Date problems after upgrading to node-sqlite3 5.0.0

[joshkel]

After upgrading from node-sqlite3 4.2.0 to 5.0.0, my Jest test suite started failing because JavaScript Date objects were inserted into the database as strings (e.g., "Sat Jul 11 2020 17:06:27 GMT-0400 (Eastern Daylight Time)") instead of (milli)seconds-since-the-epoch.

From digging into it, I believe that the problem is caused by Jest's use of Node VM contexts; anything that runs in a separate context has its own view of the Date constructor, causing statement.cc's "instanceof Date" logic to fail.

The simplest fix that I could find is for node-sqlite3 to use the N-API IsDate method; that puts its logic closer to 4.2's approach. The "is date" method requires N-API version 5; I'm not sure if that's a problem. (node-sqlite3 says it works in Node 11.x and 12.x; the compatibility matrix says that version 5 requires Node 12.11.x or newer.)

I assume that regexes are affected by the same problem but did not verify; I couldn't find an equivalent fix for them.

If you'd like for me to investigate further or open a PR, I'd be happy to do so.

Here's some sample code that demonstrates the problem; the two lines of output should both show numbers, but with node-sqlite3 5.0.0, the second line shows date text.

const vm = require('vm');
const sqlite3 = require('sqlite3').verbose();
const db = new sqlite3.Database(':memory:');

const contextObject = { db, console };

db.each('select ?', new Date(), console.log);
vm.runInNewContext("db.each('select ?', [new Date()], console.log);", contextObject);

[jschlight]

Thank you for your comprehensive analysis. This is an unusual case, but one that should probably be supported. I cannot see a better alternative to the one you're proposing.

The tool that builds node-sqlite3, node-pre-gyp, supports building the same source code for multiple N-API versions. Each build is made with a different value for the C/C++ NAPI_VERSION preprocessor symbol. The symbol can be examined for conditional compilation. So it would be possible to have node-sqlite3 support both N-API version 3 and 5. The current code could be thought of as something of an incomplete polyfill for the new IsDate method.

There's more detailed information here:

https://github.com/mapbox/node-pre-gyp#n-api-considerations

The node-sqlite3 project would need to decide if it wishes to support this additional complexity.

In the meantime, I can bring this question up for review by the N-API team at its meeting next week to see if there's a solution that does not require using the N-API IsDate method.

[jschlight]

This issue has been under discussion by the N-API team. The underlying cause is apparently not related to N-API specifically, but Node native addons in general so requires more in-depth analysis. In order to address this issue, I would be happy to review a PR that supports both N-API v3 and v5. It would then be up to the node-sqlite3 project to decide whether to land it or not.

[daniellockyer]

Happy to accept a PR for this 🙂