fix for #70

[dcolens]

#70

• call resume before the callback so that consumers can use pause() as expected
• send end event when data is consumed after a get

[mscdex]

I believe the original problem with this order was that 'data' events would start emitting immediately when calling .resume(). So I moved the .resume() after to allow the callback to set up 'data' events so as not to miss them. However that was probably from the node v0.8-ish days where streams weren't quite as good as they are nowadays.

[dcolens]

I’d actually find it handy to have the stream paused by default and let consumers resume it when they are ready, this is kind of cosmetic but I think many users can be confused by the current behaviour. The other change (moving return within the if block) is not cosmetic, in my case, it made the lib completely unusable, there was no way I would receive the end event without it and that broke piping.

Did

On 03 Jun 2015, at 16:47, Brian White [email protected] wrote:

In lib/connection.js #120 (comment):

       sock.resume();

•      cb(undefined, source);
  

  I believe the original problem with this order was that 'data' events would start emitting immediately when calling .resume(). So I moved the .resume() after to allow the callback to set up 'data' events so as not to miss them. However that was probably from the node v0.8-ish days where streams weren't quite as good as they are nowadays.

—
Reply to this email directly or view it on GitHub https://github.com/mscdex/node-ftp/pull/120/files#r31630219.

[mscdex]

Right, it makes sense to leave it paused nowadays, but in node v0.8 and earlier, it would have been unexpected. Perhaps making these changes, bumping the required node version, and releasing a new major would be best.

[dcolens]

yes that makes sense, for now we can still merge this one and change the
default to paused for the next release?

Did

On Wed, Jun 3, 2015 at 5:23 PM, Brian White [email protected]
wrote:

In lib/connection.js
#120 (comment):

       sock.resume();

•      cb(undefined, source);
  

Right, it makes sense to leave it paused nowadays, but in node v0.8 and
earlier, it would have been unexpected. Perhaps making these changes,
bumping the required node version, and release a new major would be best.

—
Reply to this email directly or view it on GitHub
https://github.com/mscdex/node-ftp/pull/120/files#r31634833.

[phillipgreenii]

Is this PR waiting on something to be merged? I would be more than happy to fix it if I knew what was holding it up.

[mscdex]

Is this change actually necessary?

[dcolens]

yes, definitely, without it, when end is reached the last bytes are never emitted because return is called before we emit them!!

[mscdex]

@dcolens @phillipgreenii I've removed the resume() in master. Can you try that and let me know if that's enough to make things work as you expect?

[dcolens]

The reason for the move of the return is in the commit description: "- send end event when data is consumed after a get“, i.e. with the return outside the if, the ‘end’ event does not fire after a get.

Did

On 19 Jul 2015, at 18:29, Brian White [email protected] wrote:

@dcolens @phillipgreenii I've removed the resume() in master. Can you try that and let me know if that's enough to make things work as you expect?

—
Reply to this email directly or view it on GitHub.

[dcolens]

You should really fully merge this