Skip to content

fix for #70 #120

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.

Sign up for GitHub

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

Open
dcolens wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix for #70 #120

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,8 +562,8 @@ FTP.prototype.get = function(path, zcomp, cb) {
if (!done) {
done = true;
ondone();
return;
}
Copy link
Owner

@mscdex mscdex Jul 19, 2015

Choose a reason for hiding this comment

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

Is this change actually necessary?

Copy link
Author

@dcolens dcolens Sep 19, 2016

Choose a reason for hiding this comment

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

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

return;
}
source._emit.apply(source, Array.prototype.slice.call(arguments));
};
Expand Down Expand Up @@ -614,8 +614,8 @@ FTP.prototype.get = function(path, zcomp, cb) {
// just like a 150
if (code === 150 || code === 125) {
started = true;
cb(undefined, source);
sock.resume();
cb(undefined, source);
Copy link
Owner

@mscdex mscdex Jun 3, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Author

@dcolens dcolens Jun 3, 2015

Choose a reason for hiding this comment

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

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 notifications@github.com 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.

Copy link
Owner

@mscdex mscdex Jun 3, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Author

@dcolens dcolens Jun 4, 2015

Choose a reason for hiding this comment

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

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 notifications@github.com
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.

} else {
lastreply = true;
ondone();
Expand Down