Replace all .pipe calls with stream module pipeline method

[alolis]

In order to automatically forward any errors that might occur within the pipeline, and properly clean up afterwards, the stream.pipeline module method is preferred.

Fixes: #368, #280

[tomas]

Any chance you could avoid using "newer" JS syntax (arrow functions or ...). I like to keep the code as backwards compatible as possible. :)

[alolis]

Any chance you could avoid using "newer" JS syntax (arrow functions or ...). I like to keep the code as backwards compatible as possible. :)

Yeah sure! Do you want me to use .apply/.call instead of the spread operator and pass all the arguments array to stream.pipeline? Any other preferred way?

Something like this maybe?

// Now, release the kraken!
var pipelineCallback = function(error) { 
  if (error) debug(error); 
}
var pipelineArgs = [resp].concat(pipeline).push(pipelineCallback);
stream.pipeline.apply(null, pipelineArgs);

[tomas]

Maybe a bit shorter?

function pipelineCb(err) { if (err) debug(err) }
stream.pipeline.apply(null, [resp].concat(pipeline).concat(pipelineCb));

By the way, in your code, pipelineArgs would contain the return value of push which is the number of elements in the array.

[alolis]

By the way, in your code, pipelineArgs would contain the return value of push which is the number of elements in the array.

Yeah, you are right! I didn't run the code, I was just trying to figure out what you like first :)

Making the changes.

[alolis]

By the way, the version in needle package.json is 2.7.0 but in npm says 2.8.0. Am I missing something?

The releases here also seem a bit behind.

[tomas]

Yeah I probably haven't pushed some tags from my local repo.

Code looks good! I'll run the test suite and merge if everything passes.

[alolis]

Cool! Let me know If I need to make any further changes!

Probably the tests need to run automatically here so you can have one less thing in your mind to do ;)

[tomas]

Are you able to run test/errors_spec.js successfully? I'm getting this:

  errors
    when host does not exist
      with callback
        ✓ does not throw
        ✓ callbacks an error (49ms)
        1) error should be ENOTFOUND or EADDRINFO or EAI_AGAIN
        ✓ does not callback a response (54ms)
        ✓ does not emit an error event (102ms)
      without callback
        ✓ does not throw
        2) emits end event once, with error
        3) error should be ENOTFOUND or EADDRINFO or EAI_AGAIN
        ✓ does not emit a readable event (50ms)
        ✓ does not emit an error event (101ms)
    when request times out waiting for response
      with callback
        4) aborts the request
        5) callbacks an error
        6) error should be ECONNRESET
        ✓ error should be ECONNRESET (367ms)
        ✓ does not emit an error event (351ms)
        ✓ does not emit an error event (352ms)
        without callback


  10 passing (8s)
  6 failing

[alolis]

Is there something I need in order to run the tests because if I npm run test I get:

Error: ENOENT: no such file or directory, open '/my/home/path/needle/test/keys/ssl.cert'

If you have instructions somewhere let me know so I can read them and run them locally.

EDIT: never mind, found it https://github.com/tomas/needle#testing

[alolis]

I just run ONLY the tests/errors_spec.js with describe.only and everything passes:

 errors
    when host does not exist
      with callback
        ✓ does not throw
        ✓ callbacks an error
        ✓ error should be ENOTFOUND or EADDRINFO or EAI_AGAIN
        ✓ does not callback a response
        ✓ does not emit an error event (105ms)
      without callback
        ✓ does not throw
        ✓ emits end event once, with error (206ms)
        ✓ error should be ENOTFOUND or EADDRINFO or EAI_AGAIN (204ms)
        ✓ does not emit a readable event (55ms)
        ✓ does not emit an error event (102ms)
    when request times out waiting for response
      with callback
        ✓ aborts the request (205ms)
        ✓ callbacks an error (202ms)
        ✓ error should be ECONNRESET (203ms)
        ✓ does not callback a response (205ms)
        ✓ does not emit an error event (355ms)
      without callback
        ✓ emits done event once, with error (253ms)
        ✓ aborts the request (203ms)
        ✓ error should be ECONNRESET (256ms)
        ✓ does not emit a readable event (252ms)
        ✓ does not emit an error event (102ms)

20 passing (3s)

I will run the whole suite.

[tomas]

What Node.js version?

[alolis]

What Node.js version?

v12.16.1

What are you using?

[tomas]

Tested both with v12.16.3 and v12.21.0. Maybe I'm missing something...

[alolis]

I run the previous tests on my MacOSX. I just run the errors spec on my Ubuntu 16 machine with node v12.16.1 and it passes there as well.

If I try to run the whole suite, however, it does not, even if I switch to master.

[alolis]

Hey @tomas, any luck running those tests in master?

[alolis]

ping @tomas?

[tomas]

Will check in a minute. A bit swamped last week!

[tomas]

Ok, got them working. Merging!

[tomas]

Just pushed needle@2.9.0 to npm. Thanks @alolis for your great work!

[alolis]

Awesome @tomas! I am glad I've helped! Time to upgrade my dependencies ;)