Support asynchronous question.default

[kaelzhang]

• change as few lines as possible
• try my best to fit the original coding style of whitespaces and CRLF
• leave the memleak warning of node.js which was already existing. Should I make another commit to fix the problem by adding a line of process.stdin.setMaxListeners(20) ?

[danielchatfield]

Need new line at end

[kaelzhang]

Wait a minute

[kaelzhang]

@danielchatfield Wait a minute plz.

[kaelzhang]

It's already 5:30am at my time. Gotta sleep. Dying.

[danielchatfield]

No problem, this will need some thought before merging anyway.

"leave the memleak warning of node.js which was already existing" <- how are you getting this warning, last time we had this it was an actual memory leak: #63

[kaelzhang]

The warning details

(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at ReadStream.EventEmitter.addListener (events.js:179:15)
    at ReadStream.Readable.on (_stream_readable.js:667:33)
    at new Interface (readline.js:124:11)
    at Object.exports.createInterface (readline.js:38:10)
    at Object.Interface.createInterface (/Users/Kael/Codes/Node/Inquirer.js/node_modules/readline2/index.js:39:21)
    at PromptUI.UI (/Users/Kael/Codes/Node/Inquirer.js/lib/ui/baseUI.js:23:40)
    at new PromptUI (/Users/Kael/Codes/Node/Inquirer.js/lib/ui/prompt.js:26:8)
    at Object.inquirer.prompt (/Users/Kael/Codes/Node/Inquirer.js/lib/inquirer.js:39:10)
    at Context.<anonymous> (/Users/Kael/Codes/Node/Inquirer.js/test/specs/inquirer.js:246:25)

[kaelzhang]

readline.createInterface(input, ...) uses process.stdin as input by default(actually done by 'readline2'). And several listeners will be attached to input such as 'data' and 'end'.

So is it something wrong that we forget to remove the listeners after the execution of .prompt method?

See you tomorrow.

[danielchatfield]

I need to go to bed as well. What are you actually running to produce that trace? I can't get it to give me that warning.

[danielchatfield]

When the prompt is finished this is called which calls this which removes the listeners we added and closes the readline - we even have a test to ensure that this happens properly.

I can't reproduce the trace you are getting (using your PR or the current code). Which version of node are you using?

[kaelzhang]

Maybe we should open another issue to talk about this? 'coz the current version of Inquirer.js already has this issue (Rel #115)

[SBoudrias]

Hey, sorry for the time it took to get a review here. I don't really like how the logic is handled here. Having two methods onEachPrompt and _onEachPrompt is a bit tedious.

IMO, the flow would be cleaner if the question was passed in a factory method that would handle the async complexity of setting up correctly every values of the questions before hand.

Do you can think you could handle this task?

[kaelzhang]

Sorry, i was deadly busy these days. I'll check this today.

Did you mean i should handle the question.default inside the centralized sys, i.e. prompt/base?

[kaelzhang]

The problems I met:

this.answers should be passed as the parameter of question.default. I've got the scenario:

Let's take npm init into consideration. Maybe the first question is "what is your repo clone url", then we answer 'git@github.com:SBoudrias/Inquirer.js.git'.

Then the second is "what is your username ?". We will notice that the default answer of username should be based on the repo from which we can analysis that the default value of username is 'SBoudrias'.

So, we can't handle the default value before hand, and it is impossible to do as a instance method of the inquirer.prompts[type] constructor, that we could not get the this.answers from there.

The conclusion is:

We must handle question.default in PromptUI as well as question.choices does.

[SBoudrias]

@kaelzhang Thanks for holding up, I think it is good enough then. I'll merge.

[kaelzhang]

@SBoudrias Thanks 😄 . I'll handle #125 then.