yeoman / generator

Rails-inspired generator system that provides scaffolding for your apps
http://yeoman.io
BSD 2-Clause "Simplified" License
1.2k stars 298 forks source link

Inquirer Separator renders as [Object Object] in 0.18 #714

Closed peterblazejewicz closed 9 years ago

peterblazejewicz commented 9 years ago

I'm updating a tool scaffolded with generator-generator and updated to v0.6.0 https://github.com/yeoman/generator-generator/releases/tag/v0.6.0 as it uses 0.18. I've migrated Inquirer to my generator module as it was dropped as global from yeoman-generator.

The code I've used to render list of options with separators now renders separators as option user can pick - not as user disabled option. So I have prompts of type list with options like:

message: 'Which official example use to build the project?',
      paginated: true,
      choices: [new inquirer.Separator('--- Using the framework ---'), {
        name: 'Starter template',
        value: 'starterTemplate',
        checked: true
      }, {
        name: 'Theme',
        value: 'theme',
        checked: false
      },

v.17:

  --- Using the framework ---
❯ Starter template 
  Theme 
  Grids 
  Jumbotron 

v.18

  [object Object]
❯ Starter template 
  Theme 
  Grids 
  Jumbotron 

Does current util for user prompts check for option type? https://github.com/SBoudrias/Inquirer.js/issues/87 Thanks!

SBoudrias commented 9 years ago

Probably a bug with the prompt storage feature. I'll look into it.

cc @stefanbuck

stefanbuck commented 9 years ago

@peterblazejewicz @SBoudrias yes this is a bug in the prompt store.

SBoudrias commented 9 years ago

@stefanbuck I saw you've assigned yourself, will you be able to have a fix soon? I'd like to carve a new patch release once this is fixed.

stefanbuck commented 9 years ago

The issue is caused by _.cloneDeep method see.

@SBoudrias Should I implement a custom copy method to keep the toString method from the inquirer.Separator?

SBoudrias commented 9 years ago

@stefanbuck From what I can see, a top level _.clone would be sufficient as we don't mutate deep objects; we only change the default value right?

After creating casting an array, we could just:

questions = questions.map(_.clone);
stefanbuck commented 9 years ago

Correct, just the default property will be changed. But questions can also be an array or object. So I would like to do the clone on the questions prop, okay?

questions = _.clone(questions);
SBoudrias commented 9 years ago

Thing is cloning the array won't protect the user input as it'll only create a new array (cloning an array only copy the array indexes to a new one). That's why we'll to map the array and shallow clone every items.

As we always cast the question object to an array, we can just do the map(clone) after.

peterblazejewicz commented 9 years ago

:clap: Thanks!