mokkabonna / inquirer-autocomplete-prompt

Autocomplete prompt for inquirer
ISC License
354 stars 82 forks source link

Assorted updates #134

Closed XhmikosR closed 2 years ago

XhmikosR commented 2 years ago

CI preview: https://github.com/XhmikosR/inquirer-autocomplete-prompt/actions?query=branch%3Adev

@mokkabonna let me know if you want me to move a patch to a new PR since I'm not sure if these changes should need a major/minor bump :)

Closes #133 while at it.

Better not squash and merge.

PS1. would be nice if you could drop rxjs and/or any other dependencies (without reinventing the wheel ofc, so maybe only run-async?), see #133 :) PS2. you can remove any Travis CI references in apps/integrations etc after this is merged PS3: you can probably remove lodash completely along with util.inherits later PS4: I have a major updates branch, not sure if you want to land this later since it seems to only affect devDependencies: https://github.com/XhmikosR/inquirer-autocomplete-prompt/compare/dev...dev-major

Closes #133, closes #135, closes #136 along the way

mokkabonna commented 2 years ago

Thanks a lot for the work!

I haven't been able to properly see through all changes (i have skimmed through it though and it looks clean), but when I do I will probably merge, test, release if I find nothing to object to. Give it a week or so I imagine.

Short question, is there any intentional functional changes in this PR? Seems all technical right?

XhmikosR commented 2 years ago

Yeah, most changes should be safe. That being said, if you plan to make it a major version bump due to the rxjs major update, maybe it's worth including https://github.com/XhmikosR/inquirer-autocomplete-prompt/compare/dev...dev-major too?

XhmikosR commented 2 years ago

BTW please check my OP comments and https://github.com/mokkabonna/inquirer-autocomplete-prompt/pull/134#discussion_r791940042 for later :)

EDIT: I also reverted the chalk -> picocolors change since inquirer is using chalk itself.

mokkabonna commented 2 years ago

Again, thanks alot! Good and clean commits, on point.

XhmikosR commented 2 years ago

You are welcome :)

How about https://github.com/XhmikosR/inquirer-autocomplete-prompt/compare/dev...dev-major? I think after the changes here, the next version should probably be a major one. If so, the linked patch makes sense too.

mokkabonna commented 2 years ago

I have merged dev-major in as well. And removed lodash and util.inherit. I will create a major version yes.

What patch?

Also how do you suggest I get rid of rxjs? I use it to subscribe to event.keypress etc, is there a simple fucntion I can write that can accomplish the same?

XhmikosR commented 2 years ago

All good, I just noticed you cherry picked the major deps patch too :)

Also how do you suggest I get rid of rxjs? I use it to subscribe to event.keypress etc, is there a simple fucntion I can write that can accomplish the same?

That is something I don't know TBH, otherwise I'd have submitted a PR myself. :/

mokkabonna commented 2 years ago

I could write a simple if statement guard in the onkeypress/onsubmit functions, but takewhile stops the subscription alltogether as soon as the condition is false. Which is better.

XhmikosR commented 2 years ago

If you think you can make it work that's OK, otherwise don't worry about it, at least not until inquirer removes rxjs there (since rxjs will be a dependency of inquirer already, so you only need to keep in sync to reduce the impact like I did here).