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

Fix new Buffer deprecation warning by updating dependency #552

Closed
jacobq opened this issue Aug 19, 2018 · 7 comments
Closed

Fix new Buffer deprecation warning by updating dependency #552

jacobq opened this issue Aug 19, 2018 · 7 comments

Comments

@jacobq
Copy link

jacobq commented Aug 19, 2018

Right now, it seems this project uses a very old version of inquirer. What would it take to upgrade that? The main reason I care is because this seems to be blocking resolution of an annoying deprecation warning when using yarn with Node >= 10 because it uses commitizen#inquirer#external-editor#chardet (v0.4.2, which uses new Buffer(...) -- v0.7.0 updates to Buffer.alloc(...), but it's not getting used because here inquirer is pinned to 1.2.3).

It looks like these are the breaking changes between 1.x and 6.x:

  • 2.x: Drop support for node 0.10 and 0.12
  • 3.x: Drop support for Node < 4
  • 4.x: The core codebase went through a major es5 to 6 refactor. As such, we're dropping support for Node 4. This change will likely require changes to the community Plugins as es6 classes are enforcing more restriction (like constructor can only be invoked with new)."
  • 5.x: Upgrade to RxJS v5 which updates a bunch of Reactive interface method names. For people not using the Reactive interface directly, this new major release should just work out of the box.
  • 6.x: Update to Rx.js v6
@LinusU
Copy link
Contributor

LinusU commented Aug 20, 2018

We could probably go to 3.x without problem, unfortunately going beyond that would be a breaking change from our side.

That being said, us passing inquirer to the adapter is deprecated, so we could probably create a breaking change that removes it 🙌

@karlhorky
Copy link

karlhorky commented Aug 20, 2018

In case you want to go ahead with the upgrade after all, I recently upgraded Inquirer.js from version 3 to 6 in inquirer-autocomplete-prompt. I tried to make the commits as granular as possible.

Maybe the pull request is good reference:

mokkabonna/inquirer-autocomplete-prompt#64

@LinusU
Copy link
Contributor

LinusU commented Aug 20, 2018

Nice!

The problem we have here is that we simply pass inquirer to the adapter, and thus we cannot really migrate since that code is beyond our reach 🤔

@jacobq
Copy link
Author

jacobq commented Aug 20, 2018

Thanks for considering this; I've never used inquirer and am not even sure what "the adapter" is, so I don't think I should try to dive in and make a PR unless you're for sure never going to get around to this.
A quick "band-aid" may also be possible if you could upgrade transitive dependency chardet to v0.7.0 without changing anything else (e.g. https://yarnpkg.com/lang/en/docs/selective-version-resolutions/ or https://github.com/rogeriochaves/npm-force-resolutions) as I don't think its API has changed significantly between that and its current version.

@jimthedev
Copy link
Member

jimthedev commented Aug 20, 2018 via email

@LinusU
Copy link
Contributor

LinusU commented Aug 20, 2018

prompter here is an external adapter, e.g. cz-conventional-changelog

prompter(inquirer, function (error, template, overrideOptions) {

I'd be happy with a PR that removes inquirer, and passes in an object that mimics the API, but throws a helpful error whenever it's being called, something like this:

const inquirer = {
  prompt () {
    throw new Error('Being passed an instance of `inquirer` from `cz-cli` is removed, please depend on `inquirer` directly')
  }

  // all other methods...
}

@jimthedev
Copy link
Member

Sounds good

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

No branches or pull requests

4 participants