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

inject function improvements #100

Merged
merged 1 commit into from
Dec 2, 2018
Merged

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Oct 16, 2018

This PR provide 2 improvements to the inject function:

  • Allow to define multiple answer for the same question, which allow to support repeated questions
  • Trigger all the callback that a non inject answer would which allow to test functionalities such as onSubmit or format

Here is an example of a prompt and it's associated test:

const prompts = require('prompts');
const delay = require('delay');

const questions = [{
    type: 'number',
    name: 'price',
    message: 'Guess the price',
    format: val => Intl.NumberFormat(undefined, { style: 'currency', currency: 'USD' }).format(val)
}];
const answers = [];

const ask = async questions => prompts(questions, {
   onSubmit: async (prompt, response) => {
    if (prompt.name === 'price' && !(await checkPriceOnServer(response))) {
      console.log('You guessed wrong...try again!');
      // Ask again and collect answer
      answers.push(await ask(questions));
      return;
    }
    console.log('You guessed right!');
  },
    onCancel: async prompt => {
      if (prompt.name === 'price') {
        console.log('Don\'t stop prompting until you guess right...');
        // Ask again and collect answer
        answers.push(await ask(questions));
        return true;
      }
    }
})

async function checkPriceOnServer(price) {
  // Simulate async response
  return delay(1000).then(() => price === '$10.00');
}

module.exports = async () => {
  await ask(questions);
  return Object.assign(...answers.reverse());
};
const prompts = require('prompts');
const prompter = require('./prompter');
prompts.inject({price: [5, new Error('Hit Ctrl+C'), 10]});
prompter().then(console.log);

// => You guessed wrong...try again!
// => Don't stop prompting until you guess right...
// => You guessed right!
// => { price: '$10.00' }

@terkelg
Copy link
Owner

terkelg commented Oct 16, 2018

Thanks @pvdlg! I'll take a look at this over the weekend. Looks good

@terkelg terkelg requested a review from lukeed October 20, 2018 12:23
@terkelg
Copy link
Owner

terkelg commented Oct 20, 2018

Looks good to me. What do you think @lukeed?

@lukeed
Copy link
Collaborator

lukeed commented Oct 20, 2018

Hmm.. wouldn't this break/change the responses to select and multiselect prompts? Those should be Arrays, and if I'm not mistaken, this PR will treat all Arrays as a list to cycle through, rather than accepting it's answer in array-form.

@pvdlg
Copy link
Contributor Author

pvdlg commented Oct 22, 2018

Yes indeed that would be problematic for multiselect as we would have no way to tell if an Array is a single answer with multiple values or multiple answers with a single value.

The objective was to not make a breaking change, but I overlooked that particular case.
As it seems a breaking change is inevitable, maybe we could change the API to accept only an Array of answers.

prompts.inject(['answer1', ['multiple', 'multiselect', 'answers'], 'answer3']);

Or do you have a preference for a different API?

@lukeed
Copy link
Collaborator

lukeed commented Oct 22, 2018

Sure, that could work! A lot of (deep) nested arrays can be visually confusing though, so we may want to explore a second parameter that toggles an iterate key that, when true, either rights into a second object or configures the same object with a specialty so that at runtime we know if we are iterating or using an arra value. That way we can iterate through any type of prompt

@pvdlg
Copy link
Contributor Author

pvdlg commented Oct 22, 2018

That would be only one level of nested array, so not that deep.

I don't really understand what you are suggesting...Can you provide an example of how that API would be used?

@terkelg
Copy link
Owner

terkelg commented Oct 29, 2018

Sorry I'm not to much help here. @lukeed can you elaborate for @pvdlg?

@lukeed
Copy link
Collaborator

lukeed commented Oct 29, 2018

Oh, sorry~! I typed that on the run, hence no code sample.

I think I meant this:

// Instead of:

prompts.inject({
  text: 'foo', // regular
  age: [5, new Error('Hit Ctrl+C'), 10], // must iterate
  choice: [['one', 'two']], // this should be Array-type answer
  confirm: true
]);

// Do this:

prompts.inject({
  text: 'foo', // regular
  choice: ['one', 'two'], // regular Array-type answer
  confirm: true
});

prompts.inject({
  age: [5, new Error('Hit Ctrl+C'), 10], // must iterate
}, true); //=> true == must iterate

Doesn't really matter to me – as @pvdlg pointed out, it's only one extra level of depth/nesting, so that's not a big deal. This was just an idea

@hasahmed
Copy link

Needing this functionality. Specifically to run the format callbacks when injections are used for testing. I went ahead and forked @pvdlg to use these changes, but I would like to be back to the main repo if possible.

@terkelg
Copy link
Owner

terkelg commented Nov 29, 2018

Sorry, I've been busy. @pvdlg is this ready for merge?

@pvdlg
Copy link
Contributor Author

pvdlg commented Nov 30, 2018

@terkelg it depends. I tried to avoid making a breaking change, but as @lukeed mentioned there is a case where is a breaking change (multiselect prompts).

So if we are ok with a breaking change we might simplify the API further. Instead of passing objects with the prompt type and the values we want we could just pass a list of value without having to specify the prompt type.

For example with:

prompts.inject(['abc', ['item1', 'item2', 'item3'], 10]);

The fist prompt would receive 'abc', the second one ['item1', 'item2', 'item3'] and the third one 10. That would be independent of the prompt type.

It would up to the user to determine what prompt their test is supposed to call and inject the mock data accordingly.

I think this solution is more simple both in terms of implementation and clarity for the end user, as specify the prompt id doesn't serve much purpose in the context of tests.

Let me know if you'd like to make those changes.

@terkelg
Copy link
Owner

terkelg commented Dec 1, 2018

@pvdlg agree! Breaking changes are fine - let's keep it simple

@pvdlg
Copy link
Contributor Author

pvdlg commented Dec 1, 2018

@terkelg ok I updated the PR accordingly

@terkelg
Copy link
Owner

terkelg commented Dec 2, 2018

Thank you so much @pvdlg

@terkelg terkelg merged commit 309d58b into terkelg:master Dec 2, 2018
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

Successfully merging this pull request may close these issues.

4 participants