Skip to content
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.

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

Returning multiple parameters in a callback that has been grouped #58

Open
clethrill opened this issue May 15, 2017 · 5 comments
Open

Comments

@clethrill
Copy link

clethrill commented May 15, 2017

I have this code on node v6.3.1 with step 1.0.0

var Step = require('step');

Step(
	function one() {
		doThis(0, "0", this);
	},
	function two(err, number, string) {
		console.log(err, number, string);

		var group = this.group();
		for (var i=0; i<2; i++) {
			doThis(i, String(i), group());
		}
	},
	function three(err, numbers, strings) {
		console.log(err, numbers, strings);
		if (err) process.exit(1);
		process.exit(0);
	}
);

function doThis(number, string, callback) {
	callback(null, number, string);
}

I get the following output

null 0 '0'
undefined [ 0, 1 ] undefined

So one calls do this which successfully callbacks with 0 and '0' but then I put it into a loop and use this.group() and it can no longer return the second parameter.

the expected output would be

null 0 '0'
null [ 0, 1 ] [ '0', '1' ]
@clethrill
Copy link
Author

clethrill commented May 16, 2017

I modified the library to function closer to how I believe it should behave, succeeds on all tests. Added a new test to test new funcitonality.

I have a test like this to show the old results and the new results

var fs = require('fs');
var Step = require('step');

Step(
	function one() {
		doThis(this.parallel(), 0, "0", "00");
		doThis(this.parallel(), 1, "1", "11");
	},
	function two(err, ...args) {
		console.log("two: ", err, ...args);

		var group = this.group();
		for (var i=0; i<7; i++) {
			doThis(group(), i, String(i), "pizza");
		}
	},
	function three(err, ...args) {
		console.log("three: ", err, ...args);

		var group = this.group();
		for (var i=0; i<3; i++) {
			doThis(group(), i);
		}
	},
	function four(err, ...args) {
		console.log("four: ", err, ...args);

		if (err) process.exit(1);
		process.exit(0);
	}
);

function doThis(callback, ...args) {
	fs.readFile("test.txt", function() {
		callback(null, ...args);
	});
}

old results

two: undefined [ 0, '0', '00' ] [ 1, '1', '11' ]
three: undefined [ 0, 1, 2, 3, 4, 5, 6 ]
four: undefined [ 0, 1, 2 ]

new results

two:  null  [ 0, '0', '00' ]  [ 1, '1', '11' ]
three:  null [ 0, 1, 2, 3, 4, 5, 6 ] [ '0', '1', '2', '3', '4', '5', '6' ] [ 'pizza', 'pizza', 'pizza', 'pizza', 'pizza', 'pizza', 'pizza' ]
four:  null [ 0, 1, 2 ]

@creationix
Copy link
Owner

Sorry for the slow response. I've seen this, but don't have time to review it properly. Ping me again if I don't get to it.

@clethrill
Copy link
Author

I forgot as well, but since I just ran into this issue again, I guess this is my reminder.

@creationix
Copy link
Owner

Sorry, still crazy busy. I looked at this code in years, so honestly, I'm not sure how much I can help.

Could you send this as a PR please. I don't see any reason to not add the change as long as it's not a breaking change. (Extra arguments in the callback shouldn't hurt anyone)

@clethrill
Copy link
Author

PR was done awhile ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants