Node v9.x.x compatibility

[lmm-git]

This module seems not to be compatible with Node versions >= 9.

Executing npm run test with node version 9.2.0 results in various errors:

(Part of npm run test)

root@b80709335c29:/pngjs# node_modules/tape/bin/tape test/convert-images-spec.js 
Converting images
TAP version 13
# convert sync - PngSuite.png
/usr/local/bin/node[730]: ../src/node_zlib.cc:189:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = false]: Assertion `Buffer::HasInstance(args[4])' failed.
 1: node::Abort() [node]
 2: node::Assert(char const* const (*) [4]) [node]
 3: 0x1258860 [node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 5: 0xb77f9c [node]
 6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x15e5b77042fd
Aborted (core dumped)

For me they seem to be related to nodejs/node#13322 and therefore with this commit: nodejs/node@add4b0a

There the interface used in lib/sync-inflate.js changes, therefore the first line failing is line 106 (on current master).

https://github.com/lukeapage/pngjs/blob/a3d9c92eec53de0fa3e522f43a7b4fb62831f3b0/lib/sync-inflate.js#L106

By changing line 110 and 111 to use the new internal variable names it is at least possible to circumvent the program exception (by breaking backward compatibility). But then there are several other errors which are not as easy to fix (as far as I have seen).

I'm not sure how to fix this issue by maintaining backward compatibility because there does not seem to exists an "easy" fix to support the new interface. I'm also not sure whether it is necessary to work with the internal state of zlib and not using the official API (which did not change I believe).

Probably someone of the project maintainers could take a look into it or could just shortly write how they plan to make the module ready for Node 9. Probably I am just overlooking something.

[anescobar1991]

@lukeapage could you or another maintainer advice on this one?

[LinusU]

I wish I had searched the issues before trying to track down this myself 😂

Just arrived at the same line, the call to writeSync and saw that _buffer and _offset was undefined. As you say, just changing the names gives a ton of other errors, but prevents the crash.

I experimented with trying to use the builtin inflateSync instead, and it passes for most images. For a number of them though, the chunkSize gets set to a value that seems to be too low for zlib to accept.

Removing the custom chunkSize seems to trigger another timeout when reading in the ChunkStream class. Currently looking into if there is any way to fix that...

update:

Interestingly, the following patch works fine on Node.js 6.x. So it seems like my problem in ChunkStream didn't have anything to do with these changes, but rather is yet another problem in 9.x that we need to solve...

diff --git a/lib/parser-sync.js b/lib/parser-sync.js
index a49e386..342becf 100644
--- a/lib/parser-sync.js
+++ b/lib/parser-sync.js
@@ -67,14 +67,7 @@ module.exports = function(buffer, options) {
   var inflateData = Buffer.concat(inflateDataList);
   inflateDataList.length = 0;
 
-  var inflatedData;
-  if (metaData.interlace) {
-    inflatedData = zlib.inflateSync(inflateData);
-  } else {
-    var rowSize = ((metaData.width * metaData.bpp * metaData.depth + 7) >> 3) + 1;
-    var imageSize = rowSize * metaData.height;
-    inflatedData = inflateSync(inflateData, { chunkSize: imageSize, maxLength: imageSize });
-  }
+  var inflatedData = zlib.inflateSync(inflateData);
   inflateData = null;
 
   if (!inflatedData || !inflatedData.length) {

[threehams]

A PR was merged for this, so it's just waiting on a release.
#102

[lukeapage]

Actually the Pr is published when it was merged as 3.3.2...

[threehams]

Got it! I was looking at the Github releases page, not npm.