Let the adapters depend on inquirer

[LinusU]

Instead of us supplying inquirer to the adapters, I think that it would be very nice if they could depend on it themselves. Then they can update that dependency at their own discretion instead of being dependent on the main module.

Currently you can choose to be compatible with either commitizen > 2.8.0, or with versions < 2.8.0 since we upgraded inquirer which broke the api. Since this has already been updated as well in conventional-changelog, I think it's to late to revert the update here.

The problem I'm facing as an adapter author is that if I upgrade, I need to go around and tell everyone at the office to update to a newer version of commitizen. I also must upgrade all of the repos that uses our internal adapter at the same time. Otherwise we will have some repos that works in one version, and some that works in other.

Basically this is quite a lot of pain and I think that we should move towards letting the adapters depend on their dependencies separately.

Also, there might even be a benefit to let our interface to the adapters be completely language agnostic. We could just execute the file, and use the output as the commit message.

@jimthedev I know that you are very busy but if you could just quickly answer if you are in favour of this, then I could implement it 👌

[jimthedev]

@LinusU I like the idea of using the output as the commit message. If we can find a way that is relatively agnostic of inquirer then that would be preferable. The main reason I did this was because at least in some adapters I needed to manage the whitespace and I wasn't sure how that would work exactly. I agree that having adapters and the cli have shared dependencies (even if it is just the one) doesn't really scale. I'd say go ahead and implement it and we'll release a breaking change release. We can then go through and put in PRs for the adapters we know about.

[LinusU]

I'd say go ahead and implement it and we'll release a breaking change release. We can then go through and put in PRs for the adapters we know about.

Actually, I think that the work is best done in all of the adapters first. We don't need to change anything on our part to depend on inquirer in the adapters. I just tried it out with our internal adapter at work, and it works great.

We could potentially add a deprecation warning in this repository though.

[jimthedev]

Oh right, ok let's do the adapters first then. We've done deprecation warnings before and they work pretty well.

[leonardoanalista]

Let adapters depend on Inquire#v1.#.# is the simplest solution for sure.

[pmcelhaney]

As I understand, Inquirer switched from a callback API to a promise API, which in turn changed the API of cz.prompt(). IMHO, the best solution would be to update cz.prompt() so that it can both take a callback and return a promise.

I don't have time at the moment to look for the code, but the change would be something like this:

  cz.prompt = function(optionalCallback) {
     var prompt = inquirer.prompt();
     if (optionalCallback) { 
         prompt.then(optionalCallback);   
     } 
     return prompt;
  }

I agree in the long term it's better to encourage adapters to depend on Inquirer directly (and maybe even deprecate cz.prompt() altogether), but following semver we should view this is an inadvertent breaking change and fix it in 1.8.x.

[LinusU]

That sounds like a good idea, let's 1) fix our api to accept both callback and promise, 2) migrate all adapters we know of to using own inquirer and finally 3) deprecate the cz.prompt api.

[jimthedev]

👍🏽 love it!

On June 6, 2016 at 8:03:29 AM, Linus Unnebäck (notifications@github.com)
wrote:

That sounds like a good idea, let's 1) fix our api to accept both callback
and promise, 2) migrate all adapters we know of to using own inquirer and
finally 3) deprecate the cz.prompt api.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#249 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAGpinJZLvL8H7KHrcL0Qwl3FNFt7uxMks5qJBqggaJpZM4IsYJX
.

[jimthedev]

Getting rid of inquirer will get rid of 7.1MB rxjs dependency which inquirer relies on. The adapters will still bring it in if they need it of course.

[jimthedev]

Added to 4.0.0 milestone based on this discussion: #414 (comment)