Enable json config options and non-interactive mode#1384
Conversation
raymondfeng
changed the title
| _setupGenerator() { | ||
| this.option('json', { | ||
| type: String, | ||
| description: 'Stringified JSON object to configure options', |
There was a problem hiding this comment.
It makes me wonder how practical it is to pass JSON strings via exec arguments. IIRC, there are limitations on the maximum supported length - e.g. Linux limits the length to ~100KB according to this post, Windows has a limit of 32KB according to this post.
In my experience, CLI programs usually accept large content from stdin stream.
I am proposing to slightly different set of CLI options and behavior:
# Read JSON instructions from the given file
$ lb4 app --json commands.json
# No file was provided, read instruction from stdin
$ cat commands.json | lb4 app --jsonThoughts?
| this.option('express', { | ||
| type: Boolean, | ||
| description: | ||
| 'Run the generator in express mode to skip optional questions', |
There was a problem hiding this comment.
When I first saw the name of the options, I assumed express would be related to Express.js framework.
Can we find a better option name please, for example --non-interactive or --skip-prompts?
9a48834 to
d08ae6f
Compare
274cf05 to
c96b219
Compare
db3600e to
f88f1d9
Compare
f88f1d9 to
ce31b72
Compare
| * keyword 'loopback' under 'keywords' attribute in package.json. | ||
| * 'keywords' is an array | ||
| */ | ||
| checkLoopBackProject() { |
There was a problem hiding this comment.
Don't we already have another implementation of checkLoopBackProject? I think @virkt25 was using one in his datasource work. Let's create a single implementation of this check and move it to a place where all generators can import it from.
There was a problem hiding this comment.
Hmm, all the artifact generators call super.checkLoopBackProject() which is implemented in BaseGenerator which is what this generator (ArtifactGenerator) extends. This implementation seems to be the same as the one found in the base generator so not sure why it's duplicated.
| if (process.stdin.isTTY) { | ||
| this.log( | ||
| chalk.green( | ||
| 'Please type in a json object line by line ' + |
There was a problem hiding this comment.
Uh oh, do you really expect people to type JSON by hand? What if they make a typo, e.g. forget to wrap a key in double quotes - how are they going to fix the type? Will they have to re-type everything again?
In my opinion, adding readline support is overkill. The JSON mode is intended for machines, humans should use interactive prompt mode.
There was a problem hiding this comment.
I'm thinking about copy-paste of json objects into stdin.
There was a problem hiding this comment.
Please also note the readline support is needed for unix pipe or redirection, for example:
cat config.json | lb4 app --config stdinlb4 app --config stdin < config.jsonlb4 app --config stdin <<EOF
There was a problem hiding this comment.
The generator needs to read from stdin - which is the source of input from unix pipes. The readline module is used to enable that.
There was a problem hiding this comment.
In Node.js, process.stdin is a regular stream that you can read the input from. No need to wrap it in readline, unless I am missing something specific to how Yeoman modifies stdin.
A typical solution for reading all input from stdin into a string is to pipe stdin to concat-stream.
A mock-up:
const concat = require('concat-stream');
// NOTE: returns Promise<Buffer>, not Promise<string>!
function readStream(src) {
return new Promise((resolve, reject) => {
const concatStream = concat(resolve);
src('error', reject);
src.pipe(concatStream);
});
}
// usage
const rawJsonBuffer = await readStream(stdin);
const data = JSON.parse(rawJsonBuffer);
// fortunately, JSON.parse works with buffers tooThere was a problem hiding this comment.
Well, that's just a different way of implementing readJSONFromStdin. Is leveraging readline to read from stdin a bad idea?
There was a problem hiding this comment.
The other option is the use the following code:
function readBufferFromStdin() {
const stdin = process.stdin;
const data = [];
let len = 0;
return new Promise((resolve, reject) => {
stdin.on('readable', () => {
let chunk;
while ((chunk = stdin.read())) {
data.push(chunk);
len += chunk.length;
}
});
stdin.on('end', () => {
resolve(Buffer.concat(data, len));
});
stdin.on('error', err => {
reject(err);
});
});
};There was a problem hiding this comment.
Well, that's just a different way of implementing readJSONFromStdin. Is leveraging readline to read from stdin a bad idea?
I suppose using readline is not a bad idea per se. My main objection is against the complexity of the current implementation of _readJSONFromStdin. To me, the currently proposed code requires maintainers to know a lot about readline, TTY, SIGINT, etc. and the way how all things are wired together.
Compare it with my proposed solution that's just 5 lines of relatively straightforward code.
return new Promise((resolve, reject) => {
const concatStream = concat(resolve);
src.on('error', reject);
src.pipe(concatStream);
});Your second example, using ReadableStream directly, looks better to me than the original readline-based one. But why to re-implement reading a stream into a buffer when concat-stream module already does it well? Dealing with streams is tricky and it's very easy to introduce subtle bugs that are difficult to spot by casual readers.
Not a big deal though. If you prefer a hand-written readBufferFromStdin then I am ok with that too.
| q.type === 'confirm'; // A confirmation | ||
|
|
||
| // Get the default answer for a question | ||
| const defaultAnswer = async q => { |
There was a problem hiding this comment.
async function defaultAnswer(q) {
// ...
}The implementation of this function is way too long, please refactor it by extracting bits and pieces into standalone functions.
| return defaultVal != null ? defaultVal : true; | ||
| } | ||
| if (q.type === 'list' || q.type === 'rawList') { | ||
| // Default to 1st item |
There was a problem hiding this comment.
Handling of list/rawList is a prime candidate to be extracted into a new function.
| } | ||
| } | ||
| } else if (q.type === 'checkbox') { | ||
| if (def == null) { |
There was a problem hiding this comment.
Ditto here - please extract checkbox case into a function.
bajtos
changed the title
virkt25
left a comment
virkt25
left a comment
There was a problem hiding this comment.
Great PR 👏 ! I'm excited to see this PR land in the CLI :D -- Just a few quick comments but looks great otherwise.
| * keyword 'loopback' under 'keywords' attribute in package.json. | ||
| * 'keywords' is an array | ||
| */ | ||
| checkLoopBackProject() { |
There was a problem hiding this comment.
Hmm, all the artifact generators call super.checkLoopBackProject() which is implemented in BaseGenerator which is what this generator (ArtifactGenerator) extends. This implementation seems to be the same as the one found in the base generator so not sure why it's duplicated.
| if (process.stdin.isTTY) { | ||
| this.log( | ||
| chalk.green( | ||
| 'Please type in a json object line by line ' + |
|
|
||
| this.option('skip-optional-prompts', { | ||
| type: Boolean, | ||
| alias: 'b', |
There was a problem hiding this comment.
Why is the alias for this prompt b? I'd image maybe s for skip?
| description: 'JSON file to configure options', | ||
| }); | ||
|
|
||
| this.option('skip-optional-prompts', { |
There was a problem hiding this comment.
I'm not sure how I feel about a such a long flag name from a UX perspective. Can we consider a simpler / easier to type / use / remember flag such as --skip or --skip-optional or --express (but may be confusing with Express framework) or --quick or --defaults ... anything smaller than the current flag.
There was a problem hiding this comment.
I was proposing --non-interactive, but that name has a slightly different meaning.
To be honest, I don't see much value in --skip-default-prompts unless it's used with the JSON mode. I am proposing to merge these two modes together and use the --config option to drive the behavior.
- Answers are read from JSON - use non-interactive mode (skip default prompts)
- Answers are read from UI prompts - use interactive mode (ask all prompts)
That way there is only one new CLI option to add (--config).
9b49a49 to
041b5d2
Compare
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
FWIW, there are few older comments waiting to be addressed or discussed.
| * keyword 'loopback' under 'keywords' attribute in package.json. | ||
| * 'keywords' is an array | ||
| */ | ||
| checkLoopBackProject() { |
| if (process.stdin.isTTY) { | ||
| this.log( | ||
| chalk.green( | ||
| 'Please type in a json object line by line ' + |
|
The |
041b5d2 to
af291b3
Compare
raymondfeng
changed the title
4718144 to
aaab42d
Compare
|
After switching to stream-based implementation of reading stdin, the following error is thrown: I'm not sure how to fix it at the moment. BTW, the stream based approach needs to deal with |
|
I was able to get around the issue using the following code after reading /**
* Read string from stdin
*/
exports.readStringFromStdin = function(log) {
const stdin = process.stdin;
if (stdin.isTTY && log) {
log(
chalk.green('Please type in a json object ' + '(Press <ctrl>-D to end):'),
);
}
const data = [];
let len = 0;
function onData(chunk) {
data.push(chunk);
len += chunk.length;
}
// Restore stdin
function close() {
stdin.removeListener('data', onData);
stdin.pause(); // Pause the stdin so that other prompts can happen
}
return new Promise((resolve, reject) => {
// Set up `data` handler to make it compatible with mock-stdin which is
// an old style stream which does not emit `readable` events
stdin.on('data', onData);
stdin.once('end', () => {
const buf = Buffer.concat(data, len);
close();
resolve(buf.toString('utf-8'));
});
stdin.once('error', err => {
close();
reject(err);
});
});
};@bajtos Do you really think this is better/simpler than the current code using |
Bummer
Yeah, I agree with you that it's more robust to keep the readline-based implementation then. Thank you for looking under the hood and helping us both to better understand design constraints. To me, this is another argument for not using yeoman (see #844). |
| if (err) return; | ||
| const jsonStr = lines.join('\n'); | ||
| try { | ||
| const json = JSON.parse(jsonStr); |
There was a problem hiding this comment.
Please move handling of the input outside of the code dealing with stdin.
Ideally, I'd like to see something like the following in _readJSONFromStdin:
async _readJSONFromStdin() {
const input = await readFromStdin();
try {
return JSON.parse(input);
} catch (err) {
if (!process.stdin.isTTY) {
debug(err, jsonStr);
}
throw err;
}
}There was a problem hiding this comment.
Fixed. Refactor the function into utils.
| this.log( | ||
| chalk.red('The stdin is not a terminal. No prompt is allowed.'), | ||
| ); | ||
| process.exit(1); |
There was a problem hiding this comment.
Don't exit the process please, it disrupts mocha tests. Call this.exit or throw new Error instead.
| this.log( | ||
| chalk.red('The stdin is not a terminal. No prompt is allowed.'), | ||
| ); | ||
| process.exit(1); |
There was a problem hiding this comment.
Ditto. Don't exit the process, end the generator execution only.
aaab42d to
e73a949
Compare
| if (process.stdin.isTTY && log) { | ||
| log( | ||
| chalk.green( | ||
| 'Please type in a json object line by line ' + |
There was a problem hiding this comment.
In my view, this message does not belong to readTextFromStdin as readTextFromStdin can be used to read arbitrary text, not only JSON.
I am proposing to move it back to _readJSONFromStdin and also remove the no-longer used log argument.
- Allow generator options to be passed in json format from a file, stdin, or stringified value - Allow prompts to be skipped if a prompt has default value or the corresponding anwser exists in options
e73a949 to
662696c
Compare
3 participants
The PR adds the following features:
--configfrom a json file, stdin, or stringified value--yesoption so that optional questions will be skipped with default valuesFor example,
lb4 app my-app --yes lb4 app --config config.json lb4 app --config {"name":"my-app"} cat config.json | lb4 app --config stdin lb4 app --config stdin < config.json lb4 app --config stdin << EOF > {"name":"my-app"} > EOFChecklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated